Another attempt at fixing the `collections.abc` issue for Python 3.13 #2665

DanielNoord · 2024-12-23T20:41:14Z

Well, #2658 did not work.

Let's try to fix pylint-dev/pylint#10112 again with a new fix.
It is basically the same fix as last time, but this time with some more try ... except and changing the order around. First do the normal logic, then try the frozen module logic.

Note that this targets the 3.3.x branch as I couldn't get the CI of pylint to pass from the 4.x branch. Since this needs to be back ported anyway I think that is fine.

codecov · 2024-12-23T20:56:19Z

Codecov Report

Attention: Patch coverage is 95.45455% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.19%. Comparing base (f79f2b4) to head (11cf784).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
astroid/interpreter/_import/spec.py	95.45%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2665      +/-   ##
==========================================
- Coverage   93.21%   93.19%   -0.02%     
==========================================
  Files          93       93              
  Lines       11083    11088       +5     
==========================================
+ Hits        10331    10334       +3     
- Misses        752      754       +2

Flag	Coverage Δ
linux	`93.09% <95.45%> (-0.01%)`	⬇️
pypy	`93.19% <95.45%> (-0.02%)`	⬇️
windows	`93.19% <95.45%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
astroid/interpreter/_import/spec.py	`97.24% <95.45%> (-0.41%)`	⬇️

... and 1 file with indirect coverage changes

jacobtylerwalls

In 3.3.6, the django submodule wasn't imported, but even with this PR it's still being imported.

(Also major 🙏 🙏 for devoting debugging time to this, much appreciated!)

astroid/interpreter/_import/spec.py

DanielNoord · 2024-12-23T22:09:07Z

But on 3.3.5 we did import the module right? Considering 3.3.6 also didn't handle collections properly I don't think it is the right benchmark

DanielNoord · 2024-12-23T22:14:11Z

Never mind, of course on 3.3.5 we did not import them. That was the whole issue 😅
Not trying to find frozen modules for submodules.

I have pushed another change, let's see if this works. First default to the normal behaviour, after it, try and see if we can frozen modules.

jacobtylerwalls · 2024-12-23T22:48:05Z

Note that this targets the 3.3.x branch as I couldn't get the CI of pylint to pass from the 4.x branch. Since this needs to be back ported anyway I think that is fine.

Thanks. Once we have a clean pylint run, let's rebase on main and follow the usual process. Around new year I was planning to cut an alpha of astroid and bring pylint up to date, sorry it hasn't happened yet. We already have a contributor who's ready to merge in the test fixes.

DanielNoord · 2024-12-23T22:51:59Z

Note that this targets the 3.3.x branch as I couldn't get the CI of pylint to pass from the 4.x branch. Since this needs to be back ported anyway I think that is fine.

Thanks. Once we have a clean pylint run, let's rebase on main and follow the usual process. Around new year I was planning to cut an alpha of astroid and bring pylint up to date, sorry it hasn't happened yet. We already have a contributor who's ready to merge in the test fixes.

The clean run is going now :)

I'm not sure what the usual process would be here? Can't we merge this into the maintenance branch and then resolve any issues we get with merging it into 4.x later?
That way we focus on the issue at hand (we don't have a compatible astroid version) and can worry about future versions later.

ChangeLog

jacobtylerwalls · 2024-12-23T22:55:34Z

astroid/interpreter/_import/spec.py

+                    # `find_spec` raises a ValueError for modules that are missing their `__spec__`
+                    # attributes. We don't really understand why, but this is likely caused by
+                    # us re-implementing the import behaviour but not correctly.


Let's not make this sound mysterious: I thought it was just namespace modules (or maybe pypy) that sometimes do this, and it's good that we're catching the documented exception.

I really have no clue. The only reason I know that we need to catch this is because it was part of the stack of the original issue reporter. I still don't really know how to reproduce this case.

That's fine, but let's just remove the comment. The function is documented to raise ValueError, so let's just catch it without a comment.

jacobtylerwalls · 2024-12-23T22:57:32Z

astroid/interpreter/_import/spec.py

-                spec = importlib.util.find_spec(".".join((*processed, modname)))
+                try:
+                    with warnings.catch_warnings():
+                        warnings.filterwarnings("ignore", category=Warning)


What do you think about limiting this to UserWarning for parity with #2121?

I ran into issues with DeprecationWarnings for lib2to3 in the stdlib primer for pylint which is why I had to bump this to Warning.

Alright. It seems like a little bit of a regression that we're importing stdlib modules in more cases now, but I guess we have no choice really.

Yeah.. I really don't know of any other way to fix this.

astroid/interpreter/_import/spec.py

jacobtylerwalls · 2024-12-23T23:04:20Z

I'm not sure what the usual process would be here? Can't we merge this into the maintenance branch and then resolve any issues we get with merging it into 4.x later?

My thought was:
Merge this into main (is there a blocker to doing this?)
Backport to 3.3.x and release 3.3.8.
Update the pylint-dev/pylint#10125 to 3.3.8

DanielNoord · 2024-12-23T23:13:56Z

I'm not sure what the usual process would be here? Can't we merge this into the maintenance branch and then resolve any issues we get with merging it into 4.x later?

My thought was: Merge this into main (is there a blocker to doing this?) Backport to 3.3.x and release 3.3.8. Update the pylint-dev/pylint#10125 to 3.3.8

It now targets main but that immediately causes merge conflicts...

Considering I won't have any time during Christmas I'd personally prefer to target this on the maintenance branch and just get the fix out.

astroid/interpreter/_import/spec.py

Co-authored-by: Jacob Walls <[email protected]>

jacobtylerwalls

Thanks for giving me a little time to grok this, and thanks for such a diligent follow-up. Happy new year!

jacobtylerwalls · 2024-12-23T23:50:30Z

(will reapprove after merge conflicts spruced up)

DanielNoord · 2024-12-23T23:58:36Z

astroid/interpreter/_import/spec.py

+             for suffix, type_ in ImportlibFinder._SUFFIXES:
+                 file_name = modname + suffix
+                 file_path = os.path.join(entry, file_name)
+                 if cached_os_path_isfile(file_path):


@jacobtylerwalls Bit afraid of this change... The 3.3.x branch doesn't have it so I'm not sure if this introduce more issues.

But if you approve I guess we can just try to make it work.

for more information, see https://pre-commit.ci

jacobtylerwalls

Good to know, that will help me resolve the merge conflict on the backport.

github-actions · 2024-12-24T00:38:51Z

The backport to maintenance/3.3.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-maintenance/3.3.x maintenance/3.3.x
# Navigate to the new working tree
cd .worktrees/backport-maintenance/3.3.x
# Create a new branch
git switch --create backport-2665-to-maintenance/3.3.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 f94e855268895bf995fe8eb258ea7ff7d7ccd9cf
# Push it to GitHub
git push --set-upstream origin backport-2665-to-maintenance/3.3.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-maintenance/3.3.x

Then, create a pull request where the base branch is maintenance/3.3.x and the compare/head branch is backport-2665-to-maintenance/3.3.x.

…pylint-dev#2665) * Fix issue with importing of frozen submodules (cherry picked from commit f94e855)

…ns.abc` issue for Python 3.13 (#2665) (#2666) * Fix issue with importing of frozen submodules (cherry picked from commit f94e855) * Test on Python 3.13 final * Bump CI jobs to python 3.13 --------- Co-authored-by: Daniël van Noord <[email protected]>

Pierre-Sassoulas

Thanks a lot for handling this Daniel, amazing !

DanielNoord changed the base branch from main to maintenance/3.3.x December 23, 2024 21:17

DanielNoord force-pushed the collections-fix branch from f3e472f to 0a5c16b Compare December 23, 2024 21:18

DanielNoord mentioned this pull request Dec 23, 2024

Try new branch of astroid to see if change works pylint-dev/pylint#10152

Closed

jacobtylerwalls requested changes Dec 23, 2024

View reviewed changes

astroid/interpreter/_import/spec.py Show resolved Hide resolved

Fix issue with importing of frozen submodules

5306ff5

DanielNoord closed this Dec 23, 2024

DanielNoord force-pushed the collections-fix branch from 8584096 to 50916d6 Compare December 23, 2024 22:34

DanielNoord reopened this Dec 23, 2024

DanielNoord marked this pull request as ready for review December 23, 2024 22:36

DanielNoord changed the title ~~WIP fix for 3.13 collections~~ Another attempt at fixing the collections.abc issue for Python 3.13 Dec 23, 2024

DanielNoord requested review from jacobtylerwalls and Pierre-Sassoulas December 23, 2024 22:37

jacobtylerwalls reviewed Dec 23, 2024

View reviewed changes

DanielNoord changed the base branch from maintenance/3.3.x to main December 23, 2024 23:09

DanielNoord requested a review from jacobtylerwalls December 23, 2024 23:13

jacobtylerwalls reviewed Dec 23, 2024

View reviewed changes

astroid/interpreter/_import/spec.py Outdated Show resolved Hide resolved

DanielNoord and others added 3 commits December 24, 2024 00:35

Update astroid/interpreter/_import/spec.py

c3240c0

Co-authored-by: Jacob Walls <[email protected]>

Update comments

4665d93

More comment updates

0c768c8

DanielNoord requested a review from jacobtylerwalls December 23, 2024 23:38

jacobtylerwalls previously approved these changes Dec 23, 2024

View reviewed changes

Merge branch 'main' into collections-fix

f5fe7d3

DanielNoord dismissed jacobtylerwalls’s stale review via f5fe7d3 December 23, 2024 23:52

DanielNoord requested a review from jacobtylerwalls December 23, 2024 23:52

DanielNoord added 2 commits December 24, 2024 00:54

Reorder comment to fix line length

768768e

Fix merge conflicts again

18ccd48

DanielNoord commented Dec 23, 2024

View reviewed changes

DanielNoord added the backport maintenance/3.3.x label Dec 23, 2024

DanielNoord and others added 2 commits December 24, 2024 00:59

Fix better

e9ff949

[pre-commit.ci] auto fixes from pre-commit.com hooks

11cf784

for more information, see https://pre-commit.ci

jacobtylerwalls approved these changes Dec 24, 2024

View reviewed changes

jacobtylerwalls added this to the 3.3.8 milestone Dec 24, 2024

jacobtylerwalls merged commit f94e855 into pylint-dev:main Dec 24, 2024
19 of 20 checks passed

jacobtylerwalls pushed a commit to jacobtylerwalls/astroid that referenced this pull request Dec 24, 2024

Another attempt at fixing the collections.abc issue for Python 3.13 (…

43eb6d7

…pylint-dev#2665) * Fix issue with importing of frozen submodules (cherry picked from commit f94e855)

Pierre-Sassoulas approved these changes Dec 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Another attempt at fixing the `collections.abc` issue for Python 3.13 #2665

Another attempt at fixing the `collections.abc` issue for Python 3.13 #2665

DanielNoord commented Dec 23, 2024 •

edited

Loading

codecov bot commented Dec 23, 2024 •

edited

Loading

jacobtylerwalls left a comment

DanielNoord commented Dec 23, 2024

DanielNoord commented Dec 23, 2024

jacobtylerwalls commented Dec 23, 2024

DanielNoord commented Dec 23, 2024

jacobtylerwalls Dec 23, 2024

DanielNoord Dec 23, 2024

jacobtylerwalls Dec 23, 2024

jacobtylerwalls Dec 23, 2024

DanielNoord Dec 23, 2024

jacobtylerwalls Dec 23, 2024

DanielNoord Dec 23, 2024

jacobtylerwalls commented Dec 23, 2024

DanielNoord commented Dec 23, 2024

jacobtylerwalls left a comment

jacobtylerwalls commented Dec 23, 2024

DanielNoord Dec 23, 2024

jacobtylerwalls left a comment

github-actions bot commented Dec 24, 2024

Pierre-Sassoulas left a comment

Another attempt at fixing the collections.abc issue for Python 3.13 #2665

Another attempt at fixing the collections.abc issue for Python 3.13 #2665

Conversation

DanielNoord commented Dec 23, 2024 • edited Loading

codecov bot commented Dec 23, 2024 • edited Loading

Codecov Report

jacobtylerwalls left a comment

Choose a reason for hiding this comment

DanielNoord commented Dec 23, 2024

DanielNoord commented Dec 23, 2024

jacobtylerwalls commented Dec 23, 2024

DanielNoord commented Dec 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobtylerwalls commented Dec 23, 2024

DanielNoord commented Dec 23, 2024

jacobtylerwalls left a comment

Choose a reason for hiding this comment

jacobtylerwalls commented Dec 23, 2024

Choose a reason for hiding this comment

jacobtylerwalls left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 24, 2024

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Another attempt at fixing the `collections.abc` issue for Python 3.13 #2665

Another attempt at fixing the `collections.abc` issue for Python 3.13 #2665

DanielNoord commented Dec 23, 2024 •

edited

Loading

codecov bot commented Dec 23, 2024 •

edited

Loading