Lightning 3.0 updates #727
Conversation
Codecov ReportAttention:
📢 Thoughts on this report? Let us know!. |
src/dvclive/lightning.py
Outdated
| dir: Optional[str] = None, # noqa: A002 | ||
| resume: bool = False, | ||
| report: Optional[str] = None, | ||
| save_dvc_exp: bool = False, | ||
| dvcyaml: bool = True, | ||
| cache_images: bool = False, | ||
| **kwargs, |
There was a problem hiding this comment.
Explicit kwargs initially came from #178. This is no longer an issue in newer versions of lightning, and there are CLI tests already in lightning that I added below.
| # LightningCLI tests | ||
| # Copied from https://github.com/Lightning-AI/lightning/blob/e7afe04ee86b64c76a5446088b3b75d9c275e5bf/tests/tests_pytorch/test_cli.py | ||
| class TestModel(BoringModel): | ||
| def __init__(self, foo, bar=5): | ||
| super().__init__() | ||
| self.foo = foo | ||
| self.bar = bar | ||
|
|
||
|
|
||
| def _test_logger_init_args(logger_name, init, unresolved={}): # noqa: B006 | ||
| cli_args = [f"--trainer.logger={logger_name}"] | ||
| cli_args += [f"--trainer.logger.{k}={v}" for k, v in init.items()] | ||
| cli_args += [f"--trainer.logger.dict_kwargs.{k}={v}" for k, v in unresolved.items()] | ||
| cli_args.append("--print_config") | ||
|
|
||
| out = StringIO() | ||
| with mock.patch( | ||
| "sys.argv", ["any.py"] + cli_args # noqa: RUF005 | ||
| ), redirect_stdout( # noqa: RUF100 | ||
| out | ||
| ), pytest.raises( | ||
| SystemExit | ||
| ): | ||
| LightningCLI(TestModel, run=False) |
There was a problem hiding this comment.
Once we move this whole integration to lightning, we won't have to copy all this from lightning tests.
|
Tests are failing because of an issue with pip cache? Don't know if it's related to the PR. |
|
I've cleaned up the cache in Actions -> Cached (there was one for 3.7GB !) and reran it. |
| } | ||
| if dir is not None: | ||
| self._live_init["dir"] = dir | ||
| self._live_init: Dict[str, Any] = kwargs |
There was a problem hiding this comment.
double check:
- default values in kwargs, e.g. will we still consider
dvcyamlas True if it's not specified in kwargs - might worth creating copy of kwargs, so that we 100% that's it's frozen for the external world
There was a problem hiding this comment.
Added tests for both of those cases. With the test, I didn't think it's necessary to make a copy of kwargs, and I don't see it happening in other loggers like https://github.com/Lightning-AI/lightning/blob/master/src/lightning/pytorch/loggers/comet.py, but not that strong an opinion if you really feel it's necessary.
There was a problem hiding this comment.
yeah, I think we overdid this :) kwargs creates a new dict object when it's being passed I think we don't need a test (no need to test Python I guess :) ). I was asking primarily to check that tbh
same with default values, if it's Python behavior - no need to test this
dberenbaum
force-pushed
the
lightning-3.0
branch
from
e4f4605 to
a0ddede
Compare
Fixes #726
Update lightning logger kwargs to reflect dvclive 3.0 updates and other missed changes