8000 TST: Use importlib for importing in pytest by QuLogic · Pull Request #27552 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

TST: Use importlib for importing in pytest #27552

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 17, 2024
Merged

TST: Use importlib for importing in pytest #27552

merged 1 commit into from
Jan 17, 2024

Conversation

QuLogic
Copy link
Member
@QuLogic QuLogic commented Dec 20, 2023

PR summary

This fixes the rewriting so that we get detailed variable information again, which was broken with the move to Meson (which does weird import hacking for editable installs.)

PR checklist

@QuLogic QuLogic added this to the v3.9.0 milestone Dec 20, 2023
@QuLogic
Copy link
Member Author
QuLogic commented Dec 20, 2023

Interestingly, this seems to be fine on the Minimum Versions CI, but everywhere else doesn't like it. That build uses pytest 7.0.0, and locally I have 7.2.1. The other jobs appear to be 7.4.3. I will have to investigate whether that might be the cause of the problem.

@QuLogic
Copy link
Member Author
QuLogic commented Dec 20, 2023

And on Windows, I have pytest 7.4.0 which works.

@QuLogic
Copy link
Member Author
QuLogic commented Dec 21, 2023

And updating my Windows machine to 7.4.3 also works, so it seems to be something else.

@tacaswell
Copy link
Member

If I launch the tests as

pytest -n 20

it works

If I launch the tests as

python -mpytest -n 20

it fails locally

python -I -mpytest -n 20

works.
< 8000 /div>

@QuLogic
Copy link
Member Author
QuLogic commented Dec 21, 2023

Ah right; I think what we'd want is python -P? But it wasn't available in all Python versions we support. I'm also confused why the Minimum Versions was okay with that since it doesn't run things any differently.

But to continue, do we want to:

  1. Change only CI to use importlib and direct pytest?
  2. Stop recommending python -m pytest over pytest (note their default documented invocation is with pytest, not python -m pytest)?
  3. Change the pytest options for everyone?

@tacaswell
Copy link
Member

I think we want to:

  • change the options for everyone (not getting the diffs is annoying)
  • change how we invoke the tests on CI and in our docs to just pytest
  • add a note about python -P / python -I for running with python -m

There was a big push to move everything to python -m ... but that seems to have fizzled out. The main argument for it was to avoid the risk of picking up cli tools from the underlying shell when in a conda/virtual environment. However, this seems to have been instead handled by a combination of "don't mess up", aggressively always installing pip (which had the highest risk of causing chaos!), and better tooling around managing environments.

@QuLogic QuLogic force-pushed the pytest-importmode branch from eb24233 to f2b2ee0 Compare January 3, 2024 21:31
@QuLogic QuLogic added the CI: Run cygwin Run cygwin tests on a PR label Jan 3, 2024
@QuLogic
Copy link
Member Author
QuLogic commented Jan 4, 2024

OK, now we can see that that leak test if failing as:

>       assert growth <= acceptable_memory_leakage
E       assert 3014656 <= 2000000

It appears that Cygwin broke a while ago, and I've opened pybind/pybind11#4999 for that.

@github-actions github-actions bot added topic: mpl_toolkit Documentation: devdocs files in doc/devel Documentation: API files in lib/ and doc/api labels Jan 12, 2024
@ksunden ksunden mentioned this pull request Jan 13, 2024
Closed
5 tasks
@QuLogic
TST: Use importlib for importing in pytest
88e6aeb 
This fixes the rewriting so that we get detailed variable information
again, which was broken with the move to Meson (which does weird import
hacking for editable installs.)
@ksunden ksunden merged commit 8da0df9 into matplotlib:main Jan 17, 2024
@QuLogic QuLogic deleted the pytest-importmode branch January 17, 2024 20:48
@DWesl DWesl mentioned this pull request Feb 25, 2024
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tacaswell tacaswell tacaswell approved these changes

@ksunden ksunden ksunden approved these changes

Assignees
No one assigned
Labels
CI: Run cygwin Run cygwin tests on a PR Documentation: API files in lib/ and doc/api Documentation: devdocs files in doc/devel topic: testing
Projects
None yet
Milestone
v3.9.0
Development

Successfully merging this pull request may close these issues.
3 participants
@QuLogic @tacaswell @ksunden
0