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

Use 3.11 for Github actions #6551

Merged
merged 8 commits into from Sep 30, 2022
Merged

Use 3.11 for Github actions #6551

merged 8 commits into from Sep 30, 2022

Conversation

Pierre-Sassoulas
Copy link
Member

Type of Changes

Type
✨ New feature
📜 Docs

Description

Closes #5920

@Pierre-Sassoulas Pierre-Sassoulas added Maintenance Discussion or action around maintaining pylint or the dev workflow python 3.11 labels May 8, 2022
@DanielNoord
Copy link
Collaborator

This is still broken. dill isn't 3.11 compatible yet. You can test this locally, but for some reason in Github Actions we don't fail but instead hang.

@Pierre-Sassoulas Pierre-Sassoulas added the Blocked 🚧 Blocked by a particular issue label May 8, 2022
@DanielNoord
Copy link
Collaborator

dill has released a new version but we still need an astroid update for these tests to pass.

@Pierre-Sassoulas
Copy link
Member Author

dill is still broken it seems.

@DanielNoord
Copy link
Collaborator

They are releasing at the end of June.

@hugovk
Copy link
Contributor

hugovk commented Jun 22, 2022

py311 also needs adding to tox.ini

@Pierre-Sassoulas
Copy link
Member Author

Milestone for the required release of dill : https://github.com/uqfoundation/dill/milestone/17

@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented Jun 29, 2022

Re-trying as dill 0.3.5.1 was released and it look like the milestone we had to follow is not the one I thougth.

@Pierre-Sassoulas
Copy link
Member Author

We do need the next version of dill.

@DanielNoord
Copy link
Collaborator

We also need astroid 2.12 😄

@Pierre-Sassoulas Pierre-Sassoulas added the Needs astroid update Needs an astroid update (probably a release too) before being mergable label Jun 29, 2022
@AdamWill
Copy link
Contributor

AdamWill commented Jul 11, 2022

There's an astroid 2.12.1 now, which passes all its own tests on Python 3.11. Current git HEAD dill is good with 3.11, but a release hasn't been tagged yet.

@AdamWill
Copy link
Contributor

AdamWill commented Jul 11, 2022

With a recent snapshot of dill, and astroid 2.12.1, I'm down to 43 failures in the tests when building pylint 2.14.4:

https://kojipkgs.fedoraproject.org/work/tasks/1561/89381561/build.log

edit: the ones related to deprecated modules/methods are likely downstream-specific (we exclude similar tests from 3.7 and 3.8 in our pytest command), others are more likely to also affect upstream.

@DanielNoord
Copy link
Collaborator

We are working on getting support for astroid 2.12.x in pylint, progress of which you can track in #7153.

dill seems to have missed its release date on the 27th on June. I thought about sending them a reminder, but I think they know we're waiting on their release so I don't want to bug them too much.

@AdamWill
Copy link
Contributor

OK, so with new astroid and dill and #7153 , here's some really-python3.11 issues:

  • telnetlib is deprecated in 3.11, that breaks access_attr_before_def_false_positive.py
  • asyncio.coroutine was removed in 3.11, that breaks iterable_context_py3.py
  • I think due to changes in Enum upstream, we're now getting an unexpected super-init-not-called message for enum_self_defined_member_5138

@AdamWill
Copy link
Contributor

Hmm, this is a fun one. the pylint output for the syntax_error.py test has changed, which breaks both that test and import_error (which imports it). It changed from just "invalid syntax" to "expected '('". The new error seems if anything better? But not sure why it changed, or how to allow the output to differ across Python versions (if that's desired).

@DanielNoord
Copy link
Collaborator

Hmm, this is a fun one. the pylint output for the syntax_error.py test has changed, which breaks both that test and import_error (which imports it). It changed from just "invalid syntax" to "expected '('". The new error seems if anything better? But not sure why it changed, or how to allow the output to differ across Python versions (if that's desired).

This is an annoying issue which I encountered before. We should probably merge #7097 first which deals with a similar issue and then any PR that moves syntax-error functional tests to unittests is much appreciated!

@AdamWill
Copy link
Contributor

AdamWill commented Jul 11, 2022

Thanks. I'm trying to work on fixes for the things I'm sure are python 3.11 (and not just general running-the-tests-downstream stuff, or already-fixed-since-2.14.4 stuff), but I don't know the codebase very well so I may be doing it wrong. :P I'll send a PR when I'm done.

@DanielNoord
Copy link
Collaborator

Thanks. I'm trying to work on fixes for the things I'm sure are python 3.11 (and not just general running-the-tests-downstream stuff, or already-fixed-since-2.14.4 stuff), but I don't know the codebase very well so I may be doing it wrong. :P I'll send a PR when I'm done.

Much appreciated! If you need any direction feel free to ping me, that can also be on your local fork. It's often much easier with respect to time and effort for me (or other maintainers) to comment on a WIP PR than create one ourselves.

@AdamWill
Copy link
Contributor

AdamWill commented Jul 11, 2022

Thanks! The fork is https://github.com/AdamWill/pylint/commits/py311 , with my attempts at fixes for the telnetlib and asyncio.coroutine issues so far. I'll send a PR when I'm done looking through the other few failures that are left.

edit: oh, and yeah, the syntaxerror change is indeed inherited from Python itself:

[adamw@xps13k pylint (py311)]$ python3 tests/functional/s/syntax/syntax_error.py 
  File "/home/adamw/local/pylint/tests/functional/s/syntax/syntax_error.py", line 1
    def toto # [syntax-error]
             ^^^^^^^^^^^^^^^^
SyntaxError: expected '('
[adamw@xps13k pylint (py311)]$ python3.10 tests/functional/s/syntax/syntax_error.py 
  File "/home/adamw/local/pylint/tests/functional/s/syntax/syntax_error.py", line 1
    def toto # [syntax-error]
             ^^^^^^^^^^^^^^^^
SyntaxError: invalid syntax

so I guess I'll look at your idea of moving it and see if I can do that.

@cclauss
Copy link
Contributor

cclauss commented Sep 25, 2022

My mistake. On Windoze, it is . venv\\Scripts\\activate

@cclauss
Copy link
Contributor

cclauss commented Sep 25, 2022

We need a Primer / prepare / 3.7 / Linux (pull_request) if we want Primer / Run / Run / 3.7 (pull_request) to pass.

I think that line 2 of .github/workflows/primer_run_main.yaml can be removed when comparing the URL to the current action.

@Pierre-Sassoulas
Copy link
Member Author

I actually did not dare to create our own version 😄 Amazing !

@DanielNoord
Copy link
Collaborator

@Pierre-Sassoulas I think this is ready for review 😄

I did some other clean up too. Let's use 3.11 everywhere we can, might as well help the testing efforts by checking if other jobs in our CI still pass with it.

@Pierre-Sassoulas Pierre-Sassoulas removed the Blocked 🚧 Blocked by a particular issue label Sep 29, 2022
@DanielNoord
Copy link
Collaborator

Failures are expected. Everything else passes, which is also expected. The failures for 3.11 were already fixed in astroid.

@cclauss
Copy link
Contributor

cclauss commented Sep 30, 2022

Failures expected: --> #6551 (comment)

@DanielNoord
Copy link
Collaborator

I think that line 2 of .github/workflows/primer_run_main.yaml can be removed when comparing the URL to the current action.

Which line are you referring to here?

We need a Primer / prepare / 3.7 / Linux (pull_request) if we want Primer / Run / Run / 3.7 (pull_request) to pass.

This is not completely true. We need a run on main. This is because we want main to create the venv to make sure that any commits on astroid main are "taken" by the primer run on main and then "distributed" to PRs rather than having PRs use a new version of astroid compared to the main run they are compared against.
This means that any change to the primer cache, workflow, etc. on PRs will always fail to execute as those changes need to be on pylint main first. This is (highly) annoying but I haven't come up with a good solution yet that 1) only allows main to create venvs and 2) exits gracefully on PRs when there is a discrepancy. I don't think there is a good solution for it, sadly.

@cclauss
Copy link
Contributor

cclauss commented Sep 30, 2022

Which line are you referring to here?

Line 2.

Should the failing test Primer / Run / Run / 3.7 (pull_request) be dropped?

@DanielNoord
Copy link
Collaborator

This is L2 of the file you mentioned:

https://github.com/PyCQA/pylint/blob/aca8dd546e6ec03d169eb84d7421d4a1427920ce/.github/workflows/primer_run_main.yaml#L2

I don't understand what it has to do with the rest of your comment, sorry!

Should the failing test Primer / Run / Run / 3.7 (pull_request) be dropped?

Why should it? See my explanation above, it works for all other PRs. See also:
https://github.com/PyCQA/pylint/blob/aca8dd546e6ec03d169eb84d7421d4a1427920ce/.github/workflows/primer_run_pr.yaml#L14-L18

We ignore it where possible, but these ignores are not exhaustive. So if there is any file in the PR that is not being ignored then the workflow will still run.

cclauss
cclauss previously approved these changes Sep 30, 2022
Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -8,8 +8,8 @@ on:
pull_request: ~

env:
CACHE_VERSION: 27
DEFAULT_PYTHON: "3.10"
CACHE_VERSION: 29
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Pierre-Sassoulas Btw, we could also make this a repository secret. It would mean that you need to update it in the settings, but it might be easier than having to do so in PRs every time.
Same goes for DEFAULT_PYTHON and PRE_COMMIT_CACHE.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather make it accessible for everyone for now because I don't want to freeze pylint development if I'm not available. It would help if PyCQA was more democratic and I could add you to the admin group.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, but for some reason I feel like for pylint this is different. We also struggle with match ... case statements but we still support that version according to our classifier. For astroid I feel like we don't really introspect the ast fully but for pylint we do offer our current functionality completely on 3.11.
The objectives are a bit different, if you understand what I mean. But I'll leave this to you to decide!

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you're answering the other comment about package metadata ? Let's add the classifier then.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah yeah, my bad!

doc/whatsnew/fragments/5920.other Outdated Show resolved Hide resolved
Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
@Pierre-Sassoulas
Copy link
Member Author

Shall we backport too ? Maybe release 2.16 immediately ?

@DanielNoord
Copy link
Collaborator

I have some free time today so I was thinking of tackling some regressions. Notably #7003 which is blocked by #7496.

Either way, let's not release on Friday. That screwed us a bit with 2.15.

@Pierre-Sassoulas
Copy link
Member Author

Python 3.11.0 should ship on Wednesday so why not wait until after the tests pass on that?

Because then pylint would not be ready when 3.11 is out 😄 Release before Wednesday would be nice, especially considering Daniel created our own version of dill in order to be ready on time.

@cclauss
Copy link
Contributor

cclauss commented Sep 30, 2022

My mistake -- Python 3.11 official release is planned for Monday, 2022-10-24.

@Pierre-Sassoulas Pierre-Sassoulas merged commit 644f3fd into main Sep 30, 2022
@Pierre-Sassoulas Pierre-Sassoulas deleted the add-3.11-to-CI branch September 30, 2022 19:56
orSolocate pushed a commit to orSolocate/pylint that referenced this pull request Oct 1, 2022
Use ``dill-pylint`` (our own version of dill) because dill's 0.3.6 release is taking forever.

Closes pylint-dev#5920

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Discussion or action around maintaining pylint or the dev workflow Needs astroid update Needs an astroid update (probably a release too) before being mergable python 3.11
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Python 3.11-dev to Github actions
6 participants