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

Reactivate multi-threading on MacOS wheels and update code to C++20 standards #294

Merged
merged 7 commits into from
Dec 8, 2023

Conversation

fteicht
Copy link
Collaborator

@fteicht fteicht commented Dec 6, 2023

This PR:

  • Reactivate multi-threading when building MacOS wheels, which was deactivated because of cmake not able anymore to find OpenMP on MacOs without hints on where the libomp library is ;
  • Re-separate pytests in separate processes, one per pytest folder, in order to avoid launching all the pytests in a same process and having OpenMP library version conflicts with some distributions ;
  • Move the C++ codebase to c++20 standards which most dependent packages under the cpp/sdk folder are following.

@fteicht fteicht added the bug Something isn't working label Dec 6, 2023
@fteicht fteicht requested a review from g-poveda December 6, 2023 12:54
@fteicht fteicht self-assigned this Dec 6, 2023
@nhuet
Copy link
Contributor

nhuet commented Dec 6, 2023

@fteicht Can you develop why you need to split again the tests ? (by the way you forgot the flight_planning and domains tests: that is the risk, when specifying manually the subdirectories)

@fteicht
Copy link
Collaborator Author

fteicht commented Dec 6, 2023

@fteicht Can you develop why you need to split again the tests ? (by the way you forgot the flight_planning and domains tests: that is the risk, when specifying manually the subdirectories)

@nhuet If you run pytest by calling it on the parent folder of all the tests, then it runs all the tests in a same process. Unfortunately, with some distributions and especially MacOS, different versions of OpenMP are linked against the C++ hub library provided by scikit-decide, and against deep learning libraries like tensorflow provided in the dependent pip packages. It results in a crash of the whole pytest process. It does not happen when we separate the calls to pytest among the different subfolders, because then the pytests run in different processes, each process linking against a single (different) OpenMP library.

One solution could be to write a bash script that calls the pytests on the different separate test sub-folders. Then we'd call this script in the build and release scripts. It's not as robust as a single call to pytest on the parent test folder, but it's still more robust than calling the series of pytest calls multiple times in the build and release scripts.

@nhuet
Copy link
Contributor

nhuet commented Dec 6, 2023

If I understand well, what is important is to call the cpp/ subfolders in an other process than the other directories (python/) ? Is it correct (and sufficient) ?

@fteicht
Copy link
Collaborator Author

fteicht commented Dec 7, 2023

Yes you got it right @nhuet

fteicht and others added 7 commits December 7, 2023 11:49
- Reintroduce C++-20 execution header and multi-threading
- If not present, ensure OpenMP is found on MacOS with Clang
Some libraries use different version of OpenMP which conflicts (at least
on MacOS platforms):
- scikit-decide c++ library
- ortools (used in scheduling)
- deep-learning libraries (sb3, ...)

So we need to launch them separately to avoid crashes (especially during
MacOS suite test).
@fteicht fteicht force-pushed the correct-multi-threading branch from c066382 to 772e3d6 Compare December 7, 2023 17:23
Copy link
Collaborator

@g-poveda g-poveda left a comment

Choose a reason for hiding this comment

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

lgtm !

@g-poveda g-poveda merged commit d36aa2f into airbus:master Dec 8, 2023
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants