-
Notifications
You must be signed in to change notification settings - Fork 180
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
Support newer Python versions #2557
Conversation
c32302f
to
40cb251
Compare
41f645c
to
7283c03
Compare
@avylove Can you remove 3.12-dev for now, so this change can bring 3.11 support in? And please update the versions in document too. |
I'll see if I can find time to update the docs, but why would we remove 3.12-dev? Even if it fails, it's non-blocking ( |
We can wait azure and 3.12 released, and revisit it. The pipelines should be pass or fail. optional failure is confusing. |
Pushed doc changes. |
Contributors are not expected to be familiar with CI. It looks the only blocking comments to test and merge this PR further. |
Optional workflows will not block contributors, that's why they are optional. The alternative is to make them required, which is not what we want. |
It doesn't block contributors. It's blocking me to bring this PR to be merged. Please remove 3.12 in the workflow, and I will ask Lili to do more testing, and merge this PR. |
Propose another way to ensure we don't introduce changes that will break on 3.12. I have implemented the standard way this is done. I'm open to other ideas, but you haven't provided one and evidence shows you have not addressed this adequately in the past. I suggest you take this opportunity to learn something new. |
I don't want to set an expectation on contributors, they need to be familiar with CI system. I explained many times to new contributors to fix failures in checks, so we can merge PRs. I don't want to tell them like "you should fix failures in checks, but if it's optional, it can be ignored." I really appreciate your contribution on this, and I really want to bring the 3.11 supports into LISA. But if you are insisting on bringing 3.12-dev together. I have to create another PR, and cherry pick most changes from this PR to maximize your credits. |
You still haven't addressed the issue. 3.11 support is almost 5 months past GA when it should have been supported with 3.11 beta when it was released nearly 11 months ago. You have not indicated how you intend to ensure 3.12 support is not broken like 3.11 support was. If you can not provide this, I don't see how you can hold up this PR. So I will ask you very clearly. How you intend to ensure 3.12 support is not broken by future commits? |
My point is to avoid optional checks. It's higher priority than others to me. If there is a perfect way to meet both, of course to take. BTW, I see all checks passed now. Can it be required? |
We can make it required, I don't mind. The benefit of making it optional is we don't have to block if there is a 3.12-specific issue that might take more time to investigate. I don't expect that to happen. Once it works, it usually keeps working. |
Good to know, can you remove the optional check? So we can continue to do more validation on this PR? |
exclude: | ||
# Azure installation (Win, 3.12) currently hangs due to excessive pip backtracking | ||
# Disable unit tests and retry in 2Q23 | ||
- python-version: '3.12-dev' |
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.
Is it still an issue? Did you try LISA with Azure? It's our main scenario. If so, can you file a bug to Azure SDK? We should document it in our document. Ask users not to try 3.12 with LISA explicitly, that will save their time on troubleshooting it.
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.
Yes, it's still an issue, but only on Windows. I did some digging and it seems there are two dependencies that have issues installing.
azure-identify requires portalocker which requires pywin32. pywin32 added support for 3.12 in November and are testing against it, but haven't done a release since then, so I filed a bug with them.
Pillow added 3.12 support to their repo in February and plan to role it out with Pillow 10.0 by July. At that point there should be a pre-built binary in PyPI that will work for us.
There may be other issues beyond these, but these are currently what is preventing the installation of azure dependencies on Windows.
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.
Thanks for confirming. Can we update document to say explicitly that LISA doesn't support 3.12. It doesn't need to include the details. Just avoid users to try LISA with Python 3.12, and waste their time.
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.
Do we need that? Until it's GA then it's normal to assume it's not fully supported and if a downstream user finds an issue with 3.12, then we still want them to report it so we can track it.
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.
@avylove LISA with Python 3.12 on windows
doesn't work, but LISA with Python 3.12 on Linux
works?
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.
They both work now as far as the tests go. #2740
@LiliDeng This PR looks good to me overall. Please merge it in next cycle. After merged it, please document that 3.12 is not supported so far with the issue, which mentioned by Avram. |
tested tier 1 tests against gallery image, kvm stack test, work well. will merge it next week |
This PR adds testing for Python 3.11 and Python 3.12 pre-release. Since 3.12 is pre-release, the tests are set for non-blocking with
continue-on-error
.A few changes were needed to support Python 3.11+
azure
optional dependencieslisa.search_space.RequirementMethod
inherited fromstr
andenum.Enum
and the values were used like strings.f'{RequirementMethod.generate_min_capability}'
results in'generate_min_capability'
, but in Python >=3.11 it results in'RequirementMethod.generate_min_capability'
str
rather thanRequirementMethod
, so the issue was masked to Mypy3.12 seems to work as expected on Linux and, for the example, on Windows, but the unit tests require the Azure dependencies and this causes excessive backtracking (reviewing the same dependencies multiple times) in Pip. The unit tests have been disabled for 3.12 on Windows for now and we can try again in a month or two.