Only load distribution of a name once #25296
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
uranusjr
force-pushed
the
avoid-dist-double-load
branch
from
67f8abf to
4d7cdc7
Compare
ashb
requested changes
Jul 26, 2022
|
I’ve added a test for the function. |
uranusjr
force-pushed
the
avoid-dist-double-load
branch
from
4d7cdc7 to
001d139
Compare
potiuk
approved these changes
Aug 2, 2022
|
Tests failing :( |
uranusjr
force-pushed
the
avoid-dist-double-load
branch
from
001d139 to
a2cdce1
Compare
uranusjr
commented
Aug 3, 2022
| self.dist = dist.metadata['name'] | ||
| self.dist = dist.metadata['Name'] |
There was a problem hiding this comment.
Python pacakge metadata keys are case-insensitive, I use Name (the case used by the standard) everywhere so tests are easier to write.
This mimics the import system's behavior -- package of a name in front of sys.path "shadows" the one in the back, and should avoid a package from being loaded multiple times, if its containing directory appears multiple times in sys.path. This also causes a side effect: if there are *different* plugins of the same name, previously the one later in sys.path would have been discovered, but now it wouldn't. This is a reasonable compromise to me, since loading multiple packages of one same name is never a good idea in Python, and we should have been careful to preclude the possibility to begin with.
uranusjr
force-pushed
the
avoid-dist-double-load
branch
from
a2cdce1 to
1e7b5ec
Compare
|
Looks green now. @ashb ? |
ashb
approved these changes
Aug 3, 2022
ephraimbuddy
pushed a commit
that referenced
this pull request
Aug 15, 2022
(cherry picked from commit c30dc5e)
jedcunningham
pushed a commit
to astronomer/airflow
that referenced
this pull request
Aug 15, 2022
(cherry picked from commit c30dc5e)
53 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This mimics the import system's behavior -- package of a name in front of
sys.path"shadows" the one in the back, and should avoid a package from being loaded multiple times, if its containing directory appears multiple times insys.path.This also causes a side effect: if there are different plugins of the same name, previously the one later in
sys.pathwould have been discovered, but now it wouldn't. I think this is a reasonable compromise, since loading multiple packages of one same name is never a good idea inPython in the first place, and we should have been careful to preclude the possibility to begin with.
Should fix #25271. cc @rino0601 @bmoon4