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

chore: Convert direct multiprocessing.set_start_method("forkserver") call to a pytest fixture. #4377

Merged

Conversation

Skylion007
Copy link
Collaborator

@Skylion007 Skylion007 commented Dec 1, 2022

Description

  • This PR updates the conftest logic to call multiprocessing.set_start_method in a fixture from Use multiprocessing start_method "forkserver" #4306. This should be a bit safer since it ensure it can only be called once and if we import anything from conftest for whatever reason, it won't try to run the code snippit again. This is what we should have done at the start as putting any "raw" code in conftest.py is ill advised and bugprone.
  • This doc has some really good explanation about the order the fixtures are called and when. a scope='session` is called once when pytest startups and its state is shared among all tests that include it. By adding autouse=True, it is automatically added to all tests affected by conftest.py meaning that it is run regardless of which test is run, and it is only once per session (hence scope="session"). You can also have fixtures run once per module, once per class, etc... to store global state, or to have a piece of code run once before the first instance of the class is created etc...
  • Prevents this code from being run twice through use of importlib.reload or autoreload etc from Jupyter notebooks and better documents what functions are affecting the global state before a test is run

Suggested changelog entry:

* multiprocessing_set_spawn in pytest fixture for added safety.

@Skylion007 Skylion007 requested a review from rwgk December 1, 2022 17:56
@rwgk
Copy link
Collaborator

rwgk commented Dec 1, 2022

Oh, this looks interesting! (I think it's much more than a chore!)

Do you already know if that gets rid of the repeat

DISABLED std::system_error: ODR VIOLATION DETECTED: pybind11::detail::type_caster<mrc_ns::type_mrc>: SourceLocation1="/__w/pybind11/pybind11/tests/test_type_caster_odr_guard_1.cpp:17", SourceLocation2="/__w/pybind11/pybind11/tests/test_type_caster_odr_guard_2.cpp:17"

that popped up under #4376?

@Skylion007 Skylion007 changed the title chore: convert multiprocessing set_spawn to fixture in pytest bugfix: convert multiprocessing set_spawn to fixture in pytest Dec 1, 2022
@Skylion007
Copy link
Collaborator Author

DISABLED std::system_error: ODR VIOLATION DETECTED: pybind11::detail::type_caster<mrc_ns::type_mrc>: SourceLocation1="/__w/pybind11/pybind11/tests/test_type_caster_odr_guard_1.cpp:17", SourceLocation2="/__w/pybind11/pybind11/tests/test_type_caster_odr_guard_2.cpp:17"

@rwgk I am not sure? This is just functionally the same as it was before, just prevents the code from being run twice by having it be a pytest.fixture .

@Skylion007
Copy link
Collaborator Author

All the failures are flakes. Happy to merge pending approval.

@rwgk
Copy link
Collaborator

rwgk commented Dec 1, 2022

All the failures are flakes. Happy to merge pending approval.

Please give me a moment to try if it helps getting rid of the spurious ODR VIOLATION DETECTED messages.

I believe this PR is good even if it doesn't help, but I want to try first, in case a tweak here is needed for something.

TBH I don't really understand this sentence:

This should be a bit safer since it ensure it can only be caused once and if we import anything from conftest for whatever reason, it won't try to run the code snippit again.

Did you mean "called once"?

I think I'm missing something fundamental about how fixtures work. Could you please help me (and others) understand?

My (maybe incorrect) best-guess understanding is: conftest.py is imported only once, when pytest starts up. There is no direct reference to confest in all of pybind11.

@Skylion007
Copy link
Collaborator Author

Did you mean "called once"?

Yep

This doc has some really good explanation about the order the fixtures are called and when. a scope='session` is called once when pytest startups and its state is shared among all tests that include it. By adding autouse=True, it is automatically added to all tests affected by conftest.py meaning that it is run regardless of which test is run, and it is only once per session (hence scope="session"). You can also have fixtures run once per module, once per class, etc... to store global state, or to have a piece of code run once before the first instance of the class is created etc...

@Skylion007
Copy link
Collaborator Author

My (maybe incorrect) best-guess understanding is: conftest.py is imported only once, when pytest starts up. There is no direct reference to confest in all of pybind11.

Yeah, true, but let's say you just wanted to import code snippits from it outside of a pytest context for debugging, it would still set the multiprocessing state, which may be undesirable. Fixture are also nice because they document how the state of the program is setup for each test.

@rwgk
Copy link
Collaborator

rwgk commented Dec 1, 2022

Did you mean "called once"?

Yep

This doc has some really good explanation about the order the fixtures are called and when. a scope='session` is called once when pytest startups and its state is shared among all tests that include it. By adding autouse=True, it is automatically added to all tests affected by conftest.py meaning that it is run regardless of which test is run, and it is only once per session (hence scope="session"). You can also have fixtures run once per module, once per class, etc... to store global state, or to have a piece of code run once before the first instance of the class is created etc...

Thanks, I need a moment to look.

Quick results:

  • this PR does not get rid of the ODR VIOLATION DETECTED messages
  • when running the pybind11 pytest unit tests without this PR, the "forkserver" line is executed only once. I verified that via a print to /tmp/somefile with open append.

@Skylion007
Copy link
Collaborator Author

Yeah, it should be executed once either way, this just make it more robust and it ensures its' not executed more than once by more edge case things like abusing importlib.reload etc. Regardless, this is the recommended way of doing it.

Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

Could you please work your latest explanations into the PR description before merging?

Even more importantly: update the title? (because that goes into the commit hash)
I don't think "bugfix" is appropriate.
"best practices" seem more fitting.
But I'd just say what this does in the title and carefully explain in the description.

Suggested title:

Convert direct multiprocessing.set_start_method("forkserver") call to a pytest fixture.

tests/conftest.py Outdated Show resolved Hide resolved
@Skylion007 Skylion007 changed the title bugfix: convert multiprocessing set_spawn to fixture in pytest chore: convert multiprocessing set_spawn to fixture in pytest Dec 1, 2022
@Skylion007 Skylion007 changed the title chore: convert multiprocessing set_spawn to fixture in pytest chore: Convert direct multiprocessing.set_start_method("forkserver") call to a pytest fixture. Dec 1, 2022
@Skylion007 Skylion007 merged commit e133c33 into pybind:master Dec 1, 2022
@Skylion007 Skylion007 deleted the skylion007-fix-multiprocessing-conftest branch December 1, 2022 20:15
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Dec 1, 2022
@rwgk
Copy link
Collaborator

rwgk commented Dec 1, 2022

Nice description!
I jumped in and changed "caused" to "called".
Thanks for merging. I'll try smart_holder update attempt 3 :-)

@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Dec 20, 2022
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.

None yet

3 participants