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

Use config options and auto-downloaded weights #246

Merged
merged 14 commits into from
Dec 12, 2023
Merged

Conversation

melihyilmaz
Copy link
Collaborator

Fixes two issues raised by #245:

  • File path to auto-downloaded weights in cache currently isn't provided ModelRunner when initializing a model. Fixed this by changing the output of setup_model include it.
  • Options in config file are currently disregarded when loading a model checkpoint, fixed this and also added the error message to cover cases when model architecture related parameters are mismatched between the checkpoint and config file.

@wfondrie
Copy link
Collaborator

Options in config file are currently disregarded when loading a model checkpoint, fixed this and also added the error message to cover cases when model architecture related parameters are mismatched between the checkpoint and config file.

This was to some degree intentional, so that a user did not have to know exactly the parameters that were used to initialize a model in order to load the weights. When was this causing an issue?

Copy link
Collaborator

@wfondrie wfondrie left a comment

Choose a reason for hiding this comment

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

Can you elaborate more on what this PR does and the reasoning for the changes?

Additionally, tests need to be passing before this can be approved.

casanovo/casanovo.py Show resolved Hide resolved
@melihyilmaz
Copy link
Collaborator Author

Options in config file are currently disregarded when loading a model checkpoint, fixed this and also added the error message to cover cases when model architecture related parameters are mismatched between the checkpoint and config file.

This was to some degree intentional, so that a user did not have to know exactly the parameters that were used to initialize a model in order to load the weights. When was this causing an issue?

Users might still want to modify non-architecture related parameters, like precursor_mass_tol in #245, and currently those aren't read from the config file. I think having the default config file reflect the parameters for the model for which there're released weights would be enough - as long as users don't change those they'd be good and they can still modify other parameters, they'd get an error message if they deviate from config defaults while using our weights.

@wfondrie
Copy link
Collaborator

I think having the default config file reflect the parameters for the model for which there're released weights would be enough - as long as users don't change those they'd be good and they can still modify other parameters, they'd get an error message if they deviate from config defaults while using our weights.

I don't think this is a very robust, particularly in the case when users may want to change models (for example, to a non-tryptic model). Are we always to going to re-train all of our submodels with an identical architecture to the main model?

Users might still want to modify non-architecture related parameters, like precursor_mass_tol in #245,

I think the better solution would be to explicitly support overwriting these parameters of model weights, maybe including a warning that the loaded weights do not match the parameter file.

@wsnoble
Copy link
Contributor

wsnoble commented Sep 26, 2023

I agree with Will that it would be better to use the parameters associated with the given model and to issue a warning to the user that the parameters in the provided config file do not match the given model (explicitly listing the given parameter value and the value that was used).

@melihyilmaz melihyilmaz marked this pull request as draft November 28, 2023 01:09
Copy link

codecov bot commented Nov 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e073415) 89.40% compared to head (8c88a71) 88.88%.
Report is 1 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #246      +/-   ##
==========================================
- Coverage   89.40%   88.88%   -0.52%     
==========================================
  Files          12       12              
  Lines         906      918      +12     
==========================================
+ Hits          810      816       +6     
- Misses         96      102       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@melihyilmaz
Copy link
Collaborator Author

I modified the PR to incorporate the changes @wfondrie suggested (thanks!) and added test cases.

Now we:

  • Use checkpoint file for any mismatching model/architecture parameters (e.g. num_layers etc.) in checkpoint vs. config file and issue a warning about the specific mismatches.
  • Use config file for any mismatching non-architecture related parameters (e.g. decoding, training etc.) in checkpoint vs. config file
  • As before, we rely on the config file if any parameters are missing from the checkpoint and only raise an error if checkpoint is from an older incompatible release, i.e. has incompatible architectural components.

@melihyilmaz melihyilmaz marked this pull request as ready for review November 29, 2023 01:57
Copy link
Collaborator

@wfondrie wfondrie left a comment

Choose a reason for hiding this comment

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

Looks good to me. I fixed the screenshot action that was failing.

@bittremieux bittremieux merged commit 2aed9e5 into dev Dec 12, 2023
6 of 7 checks passed
@bittremieux bittremieux deleted the fix-modelrunner-inputs branch December 12, 2023 08:32
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 this pull request may close these issues.

4 participants