clean up warnings and move some to info #631
Conversation
There was a problem hiding this comment.
Sets default log level to WARNING (@daavoo any reason we set it to INFO?)
I just don't understand what default is supposed to be and what level should be used for each thing. I have read and heard as many opinions as individuals
Downgrades warning about ignoring save_dvc_exp=True during dvc exp run to info. There's nothing wrong here nor any expected action from the user, so I don't think we need a warning.
Makes sense
| if self._inside_dvc_exp: | ||
| msg = f"Skipping dvc add {path} because `dvc exp run` is running." | ||
| path_stage = None | ||
| for stage in self._dvc_repo.index.stages: |
There was a problem hiding this comment.
Leaving as a follow-up: We should probably wrap all operations around dvc_repo to not fail when a DVC exception is raised.
| if out.fspath == str(Path(path).absolute()): | ||
| path_stage = stage | ||
| break | ||
| if not path_stage: |
There was a problem hiding this comment.
Maybe it was just my bias from knowing the previous implementation. Still, I ended up with the model tracked in Git because we are skipping the dvc add entirely, which was previously adding it to gitignore when calling dvc add.
I think we might warn but not skip the dvc add, as if we reach this point it means that the user is not tracking it in the dvc.yaml and the is no .dvc file
There was a problem hiding this comment.
Yeah, this was probably a bit overly aggressive. I'll submit a PR to keep trying dvc add unless it's already tracked in the pipeline.
Closes #630
Summary of changes:
save_dvc_exp=Trueduringdvc exp runto info. There's nothing wrong here nor any expected action from the user, so I don't think we need a warning.dvc exp run. Instead, it will show a warning to add it as a pipeline output if isn't tracked by dvc or will state (only on info) that it is already tracked.