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

{Core} Python 3.11: Remove workaround for argparse that breaks on Python 3.11 #24109

Closed
wants to merge 1 commit into from

Conversation

mdboom
Copy link

@mdboom mdboom commented Oct 5, 2022

Fixes the crash inside of argparse when setting up subparsers as described here:

#23015 (comment)

Related command

All az commands.

Description

First step on the road to Python 3.11 compatibility.

Testing Guide

In a Python 3.11 environment, run az (or azdev test, though this doesn't fix all tests yet, just the vast majority).


This checklist is used to make sure that common guidelines for a pull request are followed.

Fixes the crash inside of argparse when setting up subparsers as described here:

  Azure#23015 (comment)
@ghost ghost added the customer-reported Issues that are reported by GitHub users external to the Azure organization. label Oct 5, 2022
@ghost
Copy link

ghost commented Oct 5, 2022

Thank you for your contribution mdboom! We will review the pull request and get back to you soon.

@ghost ghost added Auto-Assign Auto assign by bot Core CLI core infrastructure labels Oct 5, 2022
@ghost ghost requested a review from yonzhan October 5, 2022 20:19
@ghost ghost assigned jiasli Oct 5, 2022
@ghost ghost added this to the Oct 2022 (2022-11-01) milestone Oct 5, 2022
@major
Copy link
Contributor

major commented Oct 5, 2022

This patch fixes my issues on Fedora 37 + Rawhide (38) with 3.11.0rc2. 👏🏻

@yonzhan
Copy link
Collaborator

yonzhan commented Oct 5, 2022

Add @jiasli for awareness

@mdboom mdboom mentioned this pull request Oct 6, 2022
7 tasks
@wangzelin007
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jiasli
Copy link
Member

jiasli commented Oct 26, 2022

Thanks a lot @mdboom for the contribution. Due to the heavy workload in this sprint, we will check this PR and make sure it is handled in the next sprint.

@jacoborus
Copy link

jacoborus commented Oct 26, 2022

Thanks a lot @mdboom for the contribution. Due to the heavy workload in this sprint, we will check this PR and make sure it is handled in the next sprint.

Hi @jiasli , this problem will affect to every Microsoft Azure client using Fedora, RHEL, or Linux distro subscribed to the Python repos in the following days, as soon as they update their machines. Please, reconsider the move to the next sprint, otherwise, all of these devs will not be able to work with Azure until they find this issue, and then downgrade Python in their machines

@jiasli
Copy link
Member

jiasli commented Oct 27, 2022

this problem will affect to every Microsoft Azure client using Fedora, RHEL, or Linux distro subscribed to the Python repos in the following days, as soon as they update their machines.

I am afraid this is not true. The RPM we built for RHEL has requires a fixed version of Python:

Red Hat Universal Base Image 8:
dockerfile: ubi
image: registry.access.redhat.com/ubi8/ubi:8.4
artifact: rpm-ubi8
python_package: python39
Red Hat Universal Base Image 9:
dockerfile: ubi
image: registry.access.redhat.com/ubi9/ubi:9.0.0
artifact: rpm-ubi9
python_package: python3.9

Our Ubuntu DEB package bundles its own Python:

wget -qO- https://www.python.org/ftp/python/$PYTHON_VERSION/Python-$PYTHON_VERSION.tgz | tar -xz -C "$PYTHON_SRC_DIR"

Also, even Python 3.10 is not available on RHEL yet. It usually takes long time for Linux distributions to support latest Python.

Previously, it usually took us 1~2 sprints (months) to support new Python versions.

Please, reconsider the move to the next sprint, otherwise, all of these devs will not be able to work with Azure until they find this issue, and then downgrade Python in their machines

This sprint Oct 2022 (2022-11-01) will end very soon. Merging this PR without complete testing is too risky.

Appreciate your understanding.

@jacoborus
Copy link

I am afraid this is not true. The RPM we built for RHEL has requires a fixed version of Python

Ok, let me rephrase it: this problem will affect to every Microsoft Azure client using Fedora, RHEL, or any Linux distro subscribed to the Python repos in the following days.

This sprint Oct 2022 (2022-11-01) will end very soon. Merging this PR without complete testing is too risky. Appreciate your understanding.

I'm sorry, but I don't understand how 2 lines of code that were already planned for this sprint since 3 weeks ago, are suddenly compromising it.

I guess it's because we're just Linux users, even if we pay for M$ Azure...

@major
Copy link
Contributor

major commented Oct 27, 2022

@jacoborus I've added the patch from the PR to the long list of patches/hacks for the Fedora azure-cli package and it seems to be working okay. The tests still need work, though.

@jacoborus
Copy link

Thanks @major

@jiasli
Copy link
Member

jiasli commented Oct 28, 2022

this problem will affect to every Microsoft Azure client using Fedora, RHEL, or any Linux distro subscribed to the Python repos in the following days.

Even though we are working on support for Fedora, the new RPMs built for RHEL 8/9 are not tested on Fedora. Azure CLI package for Fedora is mainly maintained by its community.

I'm sorry, but I don't understand how 2 lines of code that were already planned for this sprint since 3 weeks ago, are suddenly compromising it.

In Azure CLI's development process, there are far more tasks to do in order to support a new programming language version than adding 2 lines. We need to update CI/CD pipeline, thoroughly test all code path, commands and scenarios to ensure the product quality (#24494).

Meanwhile, Azure DevOps pipeline doesn't have Python 3.11 available right now:

image

which is tracked by

So we currently are unable to test our code in our CI/CD pipeline. We will surely continue when the Azure DevOps pipeline is updated to support Python 3.11.

We are working very hard to make Azure CLI a great product, and we truly appreciate your kind understanding.

@jiasli
Copy link
Member

jiasli commented Oct 31, 2022

I did more investigation and below are my findings.

Python 3.11 updated argparse's behavior:

https://docs.python.org/3.11/whatsnew/changelog.html#id50

bpo-39716: Raise an ArgumentError when the same subparser name is added twice to an argparse.ArgumentParser. This is consistent with the (default) behavior when the same option string is added twice to an ArgumentParser.

The PR for this change is python/cpython#18605

Per Git blame,

  • subparser.choices[command_verb] = command_verb was renamed from subparser.choices[command_name] = command_name by 517872a
  • subparser.choices[command_name] = command_name was introduced as early as 2a6f545

Running this line makes subparser.choices a str:str mapping.

In argparse._SubParsersAction, self.choices is the same dict object as self._name_parser_map:

https://github.com/python/cpython/blob/v3.10.8/Lib/argparse.py#L1159

        super(_SubParsersAction, self).__init__(
            ...
            choices=self._name_parser_map,

Later, in argparse._SubParsersAction.add_parser, self._name_parser_map's command_verb key is overwritten as a str:parser mapping:

https://github.com/python/cpython/blob/v3.10.8/Lib/argparse.py#L1179

self._name_parser_map[name] = parser

Before running this line:

image

After running this line:

image

So subparser.choices[command_verb] = command_verb is basically a no-op, but it starts to cause failure in Python 3.11 due to the new duplication check.

I am not sure why subparser.choices[command_verb] = command_verb was introduced by 2a6f545 6 years ago, possibly due to some Python 2 argparse's limitation?

@@ -91,8 +92,10 @@ def load_command_table(self, command_loader):

command_verb = command_name.split()[-1]
# To work around http://bugs.python.org/issue9253, we artificially add any new
Copy link
Member

Choose a reason for hiding this comment

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

I checked http://bugs.python.org/issue9253 mentioned in the comment, but didn't see how it is related to this workaround.

@jiasli
Copy link
Member

jiasli commented Oct 31, 2022

We can think it in another way. Since Python 3.11's argparse has no functionality change (only an additional check), if this line should removed for Python 3.11, it should also be removed for Python 3.10-.

@jacoborus
Copy link

In Azure CLI's development process, there are far more tasks to....

@jiasli I'm very aware of how the development process works. And still believe that a bug that potentially can waste thousands of hours of your clients worth the time. Even more when it's a bug that was there from previous versions, Python 3.11 just unveiled it

@peruzzof
Copy link

Just to comment that I am being affected by this error, I have a pipeline with this task (running in microsoft hosted agents):

  • task: UsePythonVersion@0
    inputs:
    versionSpec: '3.x'

The workaround was to change from 3.x to 3.10, so I am assuming that Python 3.11 is already supported by Microsoft and a fix is urgent.

There is any other way to prioritize this fix?

@jiasli jiasli closed this Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auto-Assign Auto assign by bot Core CLI core infrastructure customer-reported Issues that are reported by GitHub users external to the Azure organization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants