Skip to content

Conversation

@pared
Copy link
Contributor

@pared pared commented Nov 10, 2021

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™

Recent bugfix in 3.9 release: https://bugs.python.org/issue45235
Has caused a regression. There were 2 problems:

  1. we used to override cmd value for commands like run, stage and exp init. This one is on us, and while the proposed workaround does not require it, I renamed stage-related cmd into command so it does not clash with dvc {cmd} commands.
  2. The fix has caused some other problems, for example: dvc status -q would not pass -q down to the status command, while dvc -q status would. I tried resolving the problem but today was not enough. Since the bugfix in question is most likely to get scrapped I want to limit the python version to an unaffected one, and wait for another release to get pushed. Since all Python versions seem to be affected, I am limiting all versions just in case. - can't do that due to different versions released for different os-es. Lets limit 3.9 and see how fast the revert gets released.

The action point here is that we need to observe python releases and make sure to remove constraints once we know that the new version gets released. If for some reason the fix stays in the current format, we will need to research why the flags have not been passed down and fix that.

Closes: #6956

@pared pared requested a review from a team as a code owner November 10, 2021 22:36
@pared pared requested a review from efiop November 10, 2021 22:36
@pared pared changed the title 6956 argparse problem argparse: dont override cmd Nov 10, 2021
@pared pared force-pushed the 6956_argparse_problem branch from 1ca8f72 to 283444c Compare November 10, 2021 22:39
@efiop
Copy link
Contributor

efiop commented Nov 10, 2021

Great catch with -q! I guess another question is if we might be better off just putting an version check and error-ing out on 3.9.8 and not use this fix at all? Seems like you found -q/-v/--pdb not working, who knows if there is another bug hidden somewhere, so maybe better to just forbid? WDYT?

Copy link
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! πŸ™

@efiop
Copy link
Contributor

efiop commented Nov 10, 2021

From discussions in PMs: even though cmd is not causing problems in older versions, strictly speaking it is a bug, so it is still useful to fix it, no matter where next python version will move.

@efiop efiop merged commit ab56e4b into iterative:master Nov 10, 2021
@pared
Copy link
Contributor Author

pared commented Nov 10, 2021

It's worth noting here that this PR does not solve all problems.

We managed to limit the failing tests and potential bugs for users using dvc run dvc stage or dvc exp init commands.

The change included in https://bugs.python.org/issue45235 makes using our globally defined flags (eg -q, -v, --pdb, --yappi) quite hard.
For example if we want to run dvc status -q, the quiet option will not get passed into CmdStatus. If we run dvc -q status it will.

Since this fix is supposed to be scrapped I do not investigate this particular issue further. User's might face some problems with globally defined options for python 3.9.8 version.

@efiop
Copy link
Contributor

efiop commented Nov 10, 2021

Since this fix is supposed to be scrapped I do not investigate this particular issue further. User's might face some problems with globally defined options for python 3.9.8 version.

@pared Should we forbid 3.9.8 then for now? Or invest into making it work with 3.9.8?

@pared
Copy link
Contributor Author

pared commented Nov 10, 2021

@efiop

After the changes to cmd only non-passing tests are related to --quiet option - I think we can live with that.

Due to fact that the future of the new implementation is highly uncertain, I would wait with adjusting to it for the python core team move. If they will keep that change, then surely we will need to adjust. But as of today, it seems the whole change will be reverted, so we should wait and see.

@efiop
Copy link
Contributor

efiop commented Nov 11, 2021

After the changes to cmd only non-passing tests are related to --quiet option - I think we can live with that.

But it also means that it doesn't work for users, right? And some other options too? If so, it might get in the way of using dvc. Though it looks like 3.9.8 broke lots of packages anyway, so it might get removed from everywhere or patched everywhere. So we still need to either fix it or at least forbid it, so users don't think that this is our bug. That being said, neither of those will help users of already released dvc versions, so we can definitely wait.

@efiop efiop added the bugfix fixes bug label Nov 16, 2021
@pared pared deleted the 6956_argparse_problem branch January 10, 2022 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix fixes bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Possible Issue with Python 3.9.8

2 participants