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

Support newer Python versions #2557

Closed
wants to merge 6 commits into from
Closed

Conversation

avylove
Copy link
Contributor

@avylove avylove commented Jan 25, 2023

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+

  • The versions of PyGObject and Pillow needed to be upgraded for the azure optional dependencies
  • Default values for dataclass fields must be hashable. To use an unhashable type, a default_factory must be specified instead.
  • There was a bug in LISA that happened to work in Python <=3.10
    • lisa.search_space.RequirementMethod inherited from str and enum.Enum and the values were used like strings.
    • This happens to work in Python <=3.10 because f'{RequirementMethod.generate_min_capability}' results in 'generate_min_capability', but in Python >=3.11 it results in 'RequirementMethod.generate_min_capability'
    • The method this was used in has the argument defined as str rather than RequirementMethod, so the issue was masked to Mypy

3.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.

@avylove avylove marked this pull request as ready for review January 26, 2023 14:10
@avylove avylove force-pushed the 3_11_plus branch 2 times, most recently from c32302f to 40cb251 Compare February 13, 2023 14:57
@avylove avylove force-pushed the 3_11_plus branch 2 times, most recently from 41f645c to 7283c03 Compare February 27, 2023 22:04
@squirrelsc
Copy link
Member

@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.

@avylove
Copy link
Contributor Author

avylove commented Mar 16, 2023

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 (continue-on-error: true)

@squirrelsc
Copy link
Member

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 (continue-on-error: true)

We can wait azure and 3.12 released, and revisit it. The pipelines should be pass or fail. optional failure is confusing.

@avylove
Copy link
Contributor Author

avylove commented Mar 16, 2023

Pushed doc changes.
Optional pipelines are standard and intended for exactly use cases like this. They are not confusing for anyone familiar with any of the major CI platforms.
There's no good reason to defer 3.12. Waiting for GA is way too late. It already works and now we can avoid introducing changes that will break it.

@squirrelsc
Copy link
Member

Optional pipelines are standard and intended for exactly use cases like this. They are not confusing for anyone familiar with any of the major CI platforms. There's no good reason to defer 3.12. Waiting for GA is way too late. It already works and now we can avoid introducing changes that will break it.

Contributors are not expected to be familiar with CI. It looks the only blocking comments to test and merge this PR further.

@avylove
Copy link
Contributor Author

avylove commented Mar 16, 2023

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.

@squirrelsc
Copy link
Member

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.

@avylove
Copy link
Contributor Author

avylove commented Mar 16, 2023

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.

@squirrelsc
Copy link
Member

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.

@avylove
Copy link
Contributor Author

avylove commented Mar 16, 2023

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?

@squirrelsc
Copy link
Member

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?

@avylove
Copy link
Contributor Author

avylove commented Mar 17, 2023

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.

@squirrelsc
Copy link
Member

We can make it required, I don't mind.

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'
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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?

Copy link
Contributor Author

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

@squirrelsc
Copy link
Member

@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.

@LiliDeng
Copy link
Collaborator

tested tier 1 tests against gallery image, kvm stack test, work well. will merge it next week

@LiliDeng
Copy link
Collaborator

LiliDeng commented Apr 4, 2023

@avylove I raise a new PR #2689 which includes all your changes + fix for new merged code in RequirementMethod part, close this one, thank you.

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.

3 participants