Skip to content
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

dvc: redefine is_callback as cmd and no outs/deps #5187

Merged
merged 1 commit into from
Jan 14, 2021

Conversation

efiop
Copy link
Contributor

@efiop efiop commented Dec 31, 2020

Old cmd + no deps definiton has been obsoleted since 1.0 in favor
of --always-changed and doesn't affect our ability to read history for
metrics/plots/etc so we are safe to drop the support for 2.0 onwards.

Fixes #1407

We don't have any mention of callbacks since 1.0 so no changes for docs required.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

@efiop efiop changed the title dvc: drop callback logic [WIP] dvc: drop callback logic Dec 31, 2020
@efiop efiop force-pushed the remove_is_callback branch 2 times, most recently from b132c3b to bf609d6 Compare December 31, 2020 04:56
@efiop efiop force-pushed the remove_is_callback branch from bf609d6 to 989e5f2 Compare December 31, 2020 05:21
@efiop
Copy link
Contributor Author

efiop commented Dec 31, 2020

Though this might still be useful for cmd-only stages. Or maybe we should error-out in that case and require always_changed to be set, as cmd-only stage is probably a mistake in most cases 🤔

Errors or warnings might be distracting. Changing the definition to cmd + no deps or outs is an explicit way to handle this(yet useless), but we might add an error or a warning at stage creation though.

@efiop efiop force-pushed the remove_is_callback branch from 989e5f2 to ba5af01 Compare January 6, 2021 07:01
@efiop efiop changed the title [WIP] dvc: drop callback logic [WIP] dvc: redefine is_callback as cmd and no outs/deps Jan 6, 2021
@efiop efiop force-pushed the remove_is_callback branch from ba5af01 to b5291ca Compare January 6, 2021 08:00
@efiop
Copy link
Contributor Author

efiop commented Jan 6, 2021

Thinking some more about it, it makes sense to get rid of is_callback, but actually forbid cmd with no deps/outs, as that is a 100% an error.

@efiop efiop force-pushed the remove_is_callback branch from b5291ca to 2a277c1 Compare January 13, 2021 17:55
@efiop
Copy link
Contributor Author

efiop commented Jan 13, 2021

The harmful behavior was that having no deps caused always_changed-like behavior, which could often be undesired. The case where we don't have outs or deps is still a corner-case though, where we can assume either that such stage is always changed or always unchanged. Both are pretty much useless, but the former one makes more sense to me and that is something that I would expect from dvc. We could consider forbidding dvc stage add with no deps/outs later though, but I don't remember people running into this for a long time now, so probably not worth bothering with.

@efiop efiop changed the title [WIP] dvc: redefine is_callback as cmd and no outs/deps dvc: redefine is_callback as cmd and no outs/deps Jan 13, 2021
@efiop efiop requested a review from a team January 13, 2021 19:47
@efiop efiop merged commit 3e42e01 into iterative:master Jan 14, 2021
@efiop efiop added skip-changelog Skips changelog and removed skip-changelog Skips changelog labels Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

run: abandon callback stage feature in favor of --always-changed
1 participant