CareamistV2: initialization cleanup#744
Conversation
melisande-c
left a comment
There was a problem hiding this comment.
Looks good, just a couple things.
Main thing is when loading from a checkpoint that was saved via the Lightning API but without CareamicsCheckpointInfo callback, so that there will be no experiment name saved, maybe we want to find a way that it can still be loaded by the CAREamist for predicting
| "CAREamics callback `CareamicsCheckpointInfo`. " | ||
| "Please use a checkpoint saved with CAREamics or initialize with a config instead." |
There was a problem hiding this comment.
Now that Joran merged #725 there is a default TrainingConfig so the configuration doesn't need it to be initialised. So now all that is missing is the experiment name.
As we were discussing before, if someone is trying to load a checkpoint for prediction but it doesn't have an experiment name, this is probably fine? Not sure what a good solution is though
Sorry if I didn't write this very clearly
|
@melisande-c @jdeschamps So following Milly's comment about checkpoint loading, I tried loading a checkpoint made with the current Careamist and ran into a few things I want to discuss. |
So it is a bit unfortunate that all the config gets split up in the checkpoint this way, but I was trying to follow more closely how lightning expects a checkpoint to be organised. I did consider duplicating the entire config in a different checkpoint key, but ultimately thought it was unnecessary if we know where to extract the different parts of the config.
This is partly why I want to go with calling It does mean CAREamistV2 is not compatible with old checkpoints... but we have made changes in the past which make checkpoints incompatible, namely changing the model architecture. We should definitely discuss this further though |
Opened an issue for this - #753, feel free to add thoughts there! |
Go ahead :) |
Description
Note
tldr: clean up
CAREamistV2.__init__implementationPlease ensure your PR meets the following requirements: