LightningCLI: incorrect default value of kwarg used #18616
Comments
adamjstewart
added
bug
|
To try to debug if this was a jsonargparse bug or a lightning bug, I tried using: from jsonargparse import CLI
from torchgeo.datamodules import *
if __name__ == "__main__":
#CLI(SeasonalContrastS2DataModule)
CLI(So2SatDataModule)For both data modules, jsonargparse detects the correct types or ignores the kwargs altogether if it can't. And it doesn't seem to inject fake default values when loading a |
|
Also, the problem still exists when I replace jsonargparse with omegaconf a la this tutorial. |
awaelchli
added
lightningcli
I was not able to reproduce this. The type for from jsonargparse import ArgumentParser
from torchgeo.datamodules import So2SatDataModule
parser = ArgumentParser()
parser.add_class_arguments(So2SatDataModule, 'data')
parser.print_help()Even when running the unit test that failed in
Resolving a default from code like in seco.py#L36 is currently not supported. Though, getting the default is not strictly necessary. The actual problem is a bug in jsonargparse, specifically in line _parameter_resolvers.py#L653. I will fix this.
Not checking the type is for unresolved-parameters.
That does not replace jsonargparse with omegaconf. It relpaces pyyaml with omegaconf. In both cases jsonargparse is used. |
To clarify I think the issue is that it's reading
This part surprises me. It fails in CI and it fails locally for me. I'm not sure what would be different in your environment that would cause the test to pass. Maybe a development version of something?
Thanks! |
This is now fixed and it is part of jsonargparse 4.25.0 which has just been released.
What is the motivation for doing |
I can confirm that this fixes the issue, thanks for the quick fix and release!
All of our data modules are wrappers around our datasets. So as to avoid documenting parameters twice, we keep all dataset parameters as kwargs. It just so happens that this datamodule needs access to one of the dataset parameters. If we make this a formal parameter of the datamodule, we can avoid this issue, but then we need to manually add it to kwargs. I'm not opposed to this workaround though, might do this in the meantime if we can't fix this bug.
That's the weird part, it is For some reason jsonargparse/lightning is casting it to an int, which causes the aforementioned bug. |
One last thing that needs to change. Can this constraint be relaxed? Also, this line might need to be updated if the min is now 4.18. |
|
I agree with both suggestions @adamjstewart. Would you like to open a PR applying them? |
|
@adamjstewart a small comment about your torchgeo pull request. Using as requirement |
|
Yeah, I'm torn about which to use. We also use tensorboard, and may also use omegaconf/hydra in the future. I don't love adding a dep on jsonargparse because we don't actually use it in torchgeo itself, but that would help minimize deps. |
I now managed to reproduce the issue, and would be considered a bug in jsonargparse. Though, I am not sure how to fix it. Independent from this, note that people will be writing the configs, and might be probable that someone writes |
|
Pretty sure this was fixed a long time ago, closing. |
|
I spoke too soon, the
|
Bug description
I'm trying to port TorchGeo to LightningCLI: microsoft/torchgeo#1559
99% of things are great! Unfortunately I'm still stuck on the last 1%. If you look at the failing tests, you can see two issues:
So2SatDataModule
When parsing So2SatDataModule, jsonargparse notices that kwargs may contain a
versionparameter. If it doesn't, the default is "2". However, jsonargparse detects the type as int and uses a default value of 2. Since "2" is in the dict but 2 isn't, the dataset fails to initialize.SeasonalContrastS2DataModule
When parsing SeasonalContrastS2DataModule, jsonargparse notices that kwargs may contain a
bandsparameter. If it doesn't, the default is["B4", "B3", "B2"]. However, jsonargparse detects the type asAnyand uses a default value of None. Obviously, None is invalid.It's still not clear to me if this is a lightning issue or a jsonargparse issue, but I'm surprised that it's trying to poke into my kwargs at all. Isn't that the point of
dict_kwargs, for things that can't be type checked? Even more surprising is that even though my seasonal contrast and so2sat config.yaml files don't use these parameters, lightning is adding a default value anyway.Is this a bug in jsonarpgarse or lightning? Is there any way to disable this aggressive kwarg parsing or use of default values?
What version are you seeing the problem on?
v2.0
How to reproduce the bug
No response
Error messages and logs
No response
Environment
Current environment
More info
No response
cc @carmocca @mauvilsa
The text was updated successfully, but these errors were encountered: