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

Parallel lattice_track bugfix and help #675

Merged
merged 4 commits into from
Oct 24, 2023
Merged

Conversation

swhite2401
Copy link
Contributor

This answers #673 raised by @simoneliuzzo.
The arguments pool_size and start_method were not correctly documented for the new tracking interface.
These were also not correctly handled in the new interface, leading to possible errors when set while use_mp=False.
A variable was added to at.lattice.DConstant to set the start_method at the top level.

@swhite2401
Copy link
Contributor Author

As a side comment, the default 'spawn' is non fonctionnal on MAC architectures. This had already been discussed but is still an issue in my opinion.

Copy link
Contributor

@simoneliuzzo simoneliuzzo left a comment

Choose a reason for hiding this comment

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

thank you for this change

@lfarv
Copy link
Contributor

lfarv commented Oct 21, 2023

As a side comment, the default 'spawn' is non fonctionnal on MAC architectures. This had already been discussed but is still an issue in my opinion.

I have been working 90% on macOS for years, using 3 different machines, several macOS versions and python versions, and I have never been able to reproduce the problem. patpass is also part of the test sequence run on GitHub (with the default "spawn"), and as far as I know, it never failed.

I also googled for problems with spawn on macOS, and could not find anything looking like what @simoneliuzzo described.

So I think that the problem is in the installation of python or AT, or in some configuration. I suggest that if @simoneliuzzo can reproduce the problem, I may have a look at his setup.

Also, be aware that starting at python 3.13 (the next one), "spawn" will be the default on all platforms. This may be a sign that "fork" may disappear in some time. Better make sure that "spawn" works!

@@ -30,6 +30,7 @@ class _Dst(object):
OrbMaxIter = 20 # Max. number of iterations for orbit
omp_num_threads = int(os.environ.get('OMP_NUM_THREADS', '0'))
patpass_poolsize = multiprocessing.cpu_count()
patpass_startmethod = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you document the new patpass_startmethod option in the docstring of DConstant, a few lines later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@swhite2401
Copy link
Contributor Author

As a side comment, the default 'spawn' is non fonctionnal on MAC architectures. This had already been discussed but is still an issue in my opinion.

I have been working 90% on macOS for years, using 3 different machines, several macOS versions and python versions, and I have never been able to reproduce the problem. patpass is also part of the test sequence run on GitHub (with the default "spawn"), and as far as I know, it never failed.

I also googled for problems with spawn on macOS, and could not find anything looking like what @simoneliuzzo described.

So I think that the problem is in the installation of python or AT, or in some configuration. I suggest that if @simoneliuzzo can reproduce the problem, I may have a look at his setup.

Also, be aware that starting at python 3.13 (the next one), "spawn" will be the default on all platforms. This may be a sign that "fork" may disappear in some time. Better make sure that "spawn" works!

Well I have tried the spawn method on a MAC at home and on Linux at ESRF and both failed, I am quite surprised that it works for you in fact... you are probably the only person that know for whom it works.
It would be good to understand what you are doing differently.... this is not for this PR, I'll add an issue on this

Copy link
Contributor

@lfarv lfarv left a comment

Choose a reason for hiding this comment

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

While you are here, there is still a typo in the docstring of lattice_track, output section:

trackdata: A dictionary containinf tracking data...

@swhite2401
Copy link
Contributor Author

While you are here, there is still a typo in the docstring of lattice_track, output section:

trackdata: A dictionary containinf tracking data...

Done

@swhite2401 swhite2401 merged commit 79fd464 into master Oct 24, 2023
31 checks passed
@swhite2401 swhite2401 deleted the trackfunction_help branch October 24, 2023 09:14
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