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.

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

add unique_dir option to config.project (for script tools/train.py) #661

Merged

Conversation

jpcbertoldo
Copy link
Contributor

Description

It's a tiny functionality that makes sure that the script tools/train.py will always create a new directory to dump the results in.

If config.project.unique_dir == True, then project_path = project_path / datetime.now().strftime("%Y-%m-%d_%H-%M-%S").

It should not break the current script because it defaults to False even if the option is not present in the .yaml, falling back to the previous behavior.

Changes

  • Bug fix (non-breaking change which fixes an issue)
  • Refactor (non-breaking change which refactors the code base)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist

  • My code follows the pre-commit style and check guidelines of this project.
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing tests pass locally with my changes

Copy link
Contributor

@samet-akcay samet-akcay left a comment

Choose a reason for hiding this comment

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

Would users like this minor feature? If yes, it might be an idea to document this somewhere so it would not be a hidden gem

@samet-akcay
Copy link
Contributor

I guess this change would also be consistent with the behaviour in the new CLI.

@jpcbertoldo
Copy link
Contributor Author

Would users like this minor feature? If yes, it might be an idea to document this somewhere so it would not be a hidden gem

Of course :)

I'm just writting sketchy (i.e. only the necessary code) PRs first to get some approval before writting documentation and tests.

@samet-akcay does it look good to proceed? (in particular, do you think this needs unittest?)

Comment on lines 144 to 251

if "unique_dir" not in config.project.keys():
warn("config.project.unique_dir not found config file. Setting to False for backward compatibility")
config.project.unique_dir = False # backward compatibility

if config.project.unique_dir:
project_path = project_path / datetime.now().strftime("%Y-%m-%d_%H-%M-%S")
else:
warn(
"config.project.unique_dir is set to False. "
"This does not ensure that your results will be written in an empty directory and you may overwrite files."
)

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to place this after line 158, so that the category folder comes before the unique folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yess. Thanks.

Btw, another idea : would it make sense to name it (in addition to the datetime) a hash of the config file?

Like this you know that two folders were run with exactly the same options.

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as it doesn't lead to excessively long and unreadable folder names I'm fine with adding it. We could also store a copy of the config in the folder to make it easier to find back a specific run.

@jpcbertoldo
Copy link
Contributor Author

jpcbertoldo commented Nov 2, 2022

So, a few updates:

  1. I am computing a hash of the config object

The hash does not consider all the keys in the config in order to ignore things that only concern the run itself (like directories) and should not influence the outcomes of the optimization ("math-related stuff").

Its digest 8-byte/16-charachter long.

  1. the hash is saved in a text file in the root of the project dir

  2. category will be taken in consideration as you asked @djdameln

  3. i used the opportunity to save the config file in the project dir as well (it makes debugging easier), keeping the original and normalized version.

@jpcbertoldo jpcbertoldo force-pushed the jpcbertoldo/unique-result-dir branch from 82fbccc to f48dee9 Compare November 2, 2022 13:54
…o/anomalib into jpcbertoldo/unique-result-dir
@ashwinvaidya17
Copy link
Collaborator

Not sure if I am in favour of hashing as it adds more complexity to the code. I would suggest users to use comet or wandb as they automatically upload the configuration. This makes comparison and filtering easier.

@jpcbertoldo
Copy link
Contributor Author

It was supposed to be a tiny easy PR to pass quickly 😬. I do use wandb and i see what you mean but i see some added value in adding the hash but i don't have a strong opinion on it so Im good with whatever is decided.

@samet-akcay @djdameln do you have hard opinions on this?

@samet-akcay
Copy link
Contributor

@jpcbertoldo, my concern is the verbosity that will be added to the name of the directory, which already increases with the addition of date-time. In addition, this will be obsolete when moved to the new cli and deprecate this one. Perhaps, we could only stick to the addition of date and time.

@jpcbertoldo
Copy link
Contributor Author

Done : )

Where should the docs be updated?

@samet-akcay
Copy link
Contributor

Done : )

Where should the docs be updated?

Looks like the documentation does not have a specific section for the configuration. Not sure how exactly to document this.

Perhaps, documenting this could be skipped this time as this CLI will be deprecated

Copy link
Contributor

@djdameln djdameln left a comment

Choose a reason for hiding this comment

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

Thanks!

@jpcbertoldo
Copy link
Contributor Author

image

I think the failling check here is not related to my changes.
I only changes anomalib/config/config.py, cf screenshot.

@samet-akcay
Copy link
Contributor

@jpcbertoldo, this looks like an issue from previous commits. Have you rebased your branch?

@jpcbertoldo
Copy link
Contributor Author

image

i couldnt rebase ☝️ so i updated with normal merge

@jpcbertoldo
Copy link
Contributor Author

image

Same unrelated problem with coverage report.

I think everything else is fine.

Can we merge?

@samet-akcay samet-akcay merged commit 034953f into openvinotoolkit:main Nov 8, 2022
@jpcbertoldo jpcbertoldo deleted the jpcbertoldo/unique-result-dir branch February 9, 2024 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants