Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix custom callbacks dirpath to be the same as default root dir #2194

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Fix custom callbacks dirpath to be the same as default root dir #2194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
95234da
fix: ensure that custom callbacks are stored in default_root_dir
CarlosNacher Jun 10, 2024
1942749
fix: ensure that custom callbacks are stored in default_root_dir
CarlosNacher Jun 10, 2024
5ee83bf
Merge branch 'fix/callbacks_dirpath' of https://github.com/CarlosNach…
Jul 17, 2024
9709840
Update CHANGELOG.md
Jul 17, 2024
2d33175
Merge branch 'main' into fix/callbacks_dirpath
samet-akcay Aug 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,12 +107,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
- 🐞 Fix keys in data configs to fit AnomalibDataModule parameters by @abc-125 in https://github.com/openvinotoolkit/anomalib/pull/2032
- 🐞 Fix Export docstring in CLI by @ashwinvaidya17 in https://github.com/openvinotoolkit/anomalib/pull/2058
- 🐞 Fix UFlow links by @ashwinvaidya17 in https://github.com/openvinotoolkit/anomalib/pull/2059
- 🐞 Fix custom callbacks dirpath to be the same as default root dir by @CarlosNacher in https://github.com/openvinotoolkit/anomalib/pull/2194

### New Contributors

- @seyeon923 made their first contribution in https://github.com/openvinotoolkit/anomalib/pull/1966
- @rglkt made their first contribution in https://github.com/openvinotoolkit/anomalib/pull/1956
- @DoMaLi94 made their first contribution in https://github.com/openvinotoolkit/anomalib/pull/1847
- @CarlosNacher made their first contribution in https://github.com/openvinotoolkit/anomalib/pull/2194

**Full Changelog**: https://github.com/openvinotoolkit/anomalib/compare/v1.0.1...v1.1.0

Expand Down
10 changes: 10 additions & 0 deletions src/anomalib/engine/engine.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
# SPDX-License-Identifier: Apache-2.0

import logging
import os
from collections.abc import Iterable
from pathlib import Path
from typing import Any
Expand Down Expand Up @@ -409,6 +410,15 @@ def _setup_anomalib_callbacks(self) -> None:
"""Set up callbacks for the trainer."""
_callbacks: list[Callback] = [RichProgressBar(), RichModelSummary()]

# Update dirpath of each cached callback preppending "default_root_dir"
# (which was updated in _setup_workspace() method)
for cached_callback in self._cache.args["callbacks"]:
if hasattr(cached_callback, "dirpath"):
callback_dirpath = Path(cached_callback.dirpath)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is dirpath always the correct/fixed argument name for custom callbacks? If not, this approach may not work I'm afraid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samet-akcay I am not sure if I have correctly understood your concern correcty.
That chunk of code will work with all callbacks that have a dirpath argument. If not, it won't modify anything.
So, do all callbacks which need to be locally logged name its "dirpath_" argument dirpath? I don't know, but in lightning's callbacks docs there is only a callback (ModelCheckpoint) which has to be logged and has the dirpath argument.

Please, let me know if you think I misunderstood your question because I don't know if we are talking about the same concern.

common_prefix = Path(os.path.commonpath([str(self._cache.args["default_root_dir"]), str(callback_dirpath)]))
callback_dirpath = callback_dirpath.relative_to(common_prefix)
cached_callback.dirpath = self._cache.args["default_root_dir"] / callback_dirpath

# Add ModelCheckpoint if it is not in the callbacks list.
has_checkpoint_callback = any(isinstance(c, ModelCheckpoint) for c in self._cache.args["callbacks"])
if has_checkpoint_callback is False:
Expand Down
Loading