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

Strip out calls to nullify_format_instance #1542

Merged
merged 4 commits into from
Jan 20, 2021

Conversation

ndevenish
Copy link
Member

We had problems in the past with the behaviour of HDF5 under forking/parallel reads e.g. #35 #144 01463cc 2550f7c b19510e 36f63d6 and also #1539.

While testing a fix to centralise this check I stripped out the behaviour of nullify_format_instance and found that... I couldn't reproduce any of the problems. After some digging, it looks like in HDF5 1.10.5 this problem was fixed by:

Added a new option to enable/disable using pread/pwrite instead of
read/write in the sec2, log, and core VFDs.
This option is enabled by default when pread/pwrite are detected.

Which does reading/writing in a stateless way. So, all of these workaround hacks have become redundant.

Possibly.

All the tests I've run appear to pass - this doesn't remove the need for nullify_format_instance completely because there seem to be some vestigial problem with Lazy* formats cctbx/dxtbx#245. Does anyone have any memories/documented cases where this broke before to run tests through also? Otherwise, I'd suggest that we put it in and see if any problems come up related to it.

@codecov
Copy link

codecov bot commented Jan 12, 2021

Codecov Report

Merging #1542 (f20ca62) into master (fbf1168) will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1542      +/-   ##
==========================================
+ Coverage   66.52%   66.55%   +0.02%     
==========================================
  Files         615      615              
  Lines       69152    69136      -16     
  Branches     9553     9546       -7     
==========================================
+ Hits        46005    46012       +7     
+ Misses      21211    21191      -20     
+ Partials     1936     1933       -3     

@phyy-nx
Copy link
Member

phyy-nx commented Jan 12, 2021

Did you test it under MPI conditions? test_stills_process.py has an mpi test in it.

@phyy-nx
Copy link
Member

phyy-nx commented Jan 12, 2021

conda install -c conda-forge mpi4py openmpi
mpirun hostname # test mpi is working, should see hostname for each available rank
libtbx.pytest -n 3 test_stills_process.py --regression # all three tests should run

I did the above on master and on the nullify_pid branch and the tests pass. I presume it's ok then.

@ndevenish ndevenish merged commit 24741e4 into dials:master Jan 20, 2021
@ndevenish
Copy link
Member Author

I've merged this now, with the understanding that we should back it out quickly if it is causing any apparent problems.

@ndevenish ndevenish deleted the nullify_pid branch January 20, 2021 14:08
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.

2 participants