-
Notifications
You must be signed in to change notification settings - Fork 36
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
clean up warnings and move some to info #631
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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=True
duringdvc 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.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.