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

Adapt download_model script to fetch release assets #797

Closed
hermancollin opened this issue Apr 3, 2024 · 1 comment · Fixed by #800
Closed

Adapt download_model script to fetch release assets #797

hermancollin opened this issue Apr 3, 2024 · 1 comment · Fixed by #800

Comments

@hermancollin
Copy link
Member

Describe the problem
The download_model.py script currently takes models by downloading the whole repository and extracting the model folder. It seems we used to put the .pt checkpoint files directly in the repo, but we don't do this anymore. Instead, we put them as release assets, so this method will not work anymore.

Proposed solution
I propose we simply re-write the script. If users want to download default ivadomed models, they can always get an older version of ADS and run the script, which should still work in the future.

I already have a working script we can use here: https://github.com/axondeepseg/nn-axondeepseg/blob/main/download_models.py

Related to #765

@hermancollin
Copy link
Member Author

hermancollin commented Apr 19, 2024

One annoying behavior I noticed is related to our dual use of final and best checkpoints. (See axondeepseg/nn-axondeepseg#8) By default, nnunet looks for the final checkpoints but many of our models are the best ones. To use them, we need to specify to the nnUNetPredictor.initialize_from_trained_model_folder() function. This is a bit annoying for users and could be less exposed (i.e. in general, users shouldn't have to worry about that). Here is the behavior I suggest:

  • If the model downloaded only contained best checkpoints or only final checkpoints, we select those by default. The user does not have to specify anything.
  • If both final and best models are present, we throw an error and ask the user to disambiguate with the appropriate flag.

Note that the function mentioned above is used during inference, not when downloading the models, so we could also add a function that verifies the checkpoint type in apply_models.py.

I would actually prefer to not have a --use-best-checkpoint flag because the CLI already has a lot of options. With that being said, some flags will be useless after the nnunet migration (see #798).

@hermancollin hermancollin linked a pull request Jun 5, 2024 that will close this issue
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant