-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
LightningCLI trainer_defaults
get dumped as Python object
#18731
Comments
I think this is meant to use "logger": lazy_instance(WandbLogger, project="my-project") |
I forgot about it, I think that makes the most sense. In this case we could raise an error for all non-pimitive types and suggest to use lazy_instance? |
@mauvilsa Could that break valid use-cases of serializing non-primitive types as strings? If it's fine, should it be done in |
There is already a warning for this. Maybe for some reason it is not showing? |
It does appear to me with But for somebody who inspects their Still, if there's no valid use case for serializing as a string, it would help to be strict and make it an error |
Making it an error is problematic. Types and defaults might come from signatures in third party packages. These most of the time are not possible to change by a developer. A more viable option is that the key+value is not included in the serialization. Though, I am not sure if this would be easy to do. |
I don't think that's better because then people will wonder where it is. What if we keep the current behaviour but jsonargparse adds a YAML comment on top of the fields that were serialized with the warning information? It won't work with JSON config files though. |
Would it still be problematic if it's done in Lightning rather than jsonargparse? All the user needs to do is to change to lazy_instance when passing the trainer_defaults to the CLI object. |
Since anyway there is no valid value to set, the message could be in the value.
If it is done in Lightning and is only limited to what is in |
It should be for any |
You are right, it could be |
Bug description
When providing an object like the logger to the
trainer_defaults
, it gets dumped as a Python object to the config file, which results in an invalid config file to load back into the CLI.What version are you seeing the problem on?
v2.0, master
How to reproduce the bug
Error messages and logs
Inspect the config.yaml file in the current working directory. It contains the logger dumped as an object. It's unclear what the resolution to this could be. Ideally we would want
in the config file, but since we can't know the init args that were provided at the time of instantiation, it's hard to make this work. For the time being, it might be better to just drop the entry with a warning so that loading the config doesn't fail.
Environment
Current environment
More info
jsonargparse==4.25.0
cc @carmocca @mauvilsa
The text was updated successfully, but these errors were encountered: