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

Switch DIALS to src/ layout #2077

Merged
merged 4 commits into from
Apr 21, 2022
Merged

Switch DIALS to src/ layout #2077

merged 4 commits into from
Apr 21, 2022

Conversation

ndevenish
Copy link
Member

@ndevenish ndevenish commented Apr 13, 2022

This is a WIP branch for switching DIALS to src/ layout. This will enable it to be installed as a standard python package when being used outside the libtbx build system. Behaviour under bootstrap should remain as identical as possible, but pip install dials/ will also be possible and install DIALS as a standard site-packages/ python package. Similar to dxtbx, extension compilation isn't altered here and it's assumed is dealt with separately.

The commits in this PR:

  • Move all non-tests to src/dials 1906a31
  • Update the ecosystem files - libtbx_refresh.py, libtbx_config, SConscript, setup.py - 079f6ac. The libtbx_refresh is written in a similar style to dxtbx, and installs into the build/ folder without touching conda_base/.
  • Fixup the tests to no longer assume that they are located in dials.tests. c4b2ad5

Things to do before this is ready:

  • All tests pass in xia2, dials, dxtbx
  • All tests in xfel_regression pass. xfel compiles, but the regression tests have not been run yet.
  • Testing DIALS autocomplete
  • Discussion of migration strategies

Once this is merged, I will go through and assist in rebasing/remerging any outstanding PRs. This is probably relatively simple, as git is pretty good at detecting changes to moved folders, though github is less helpful.

Transitioning to src/

A libtbx.refresh will definitely be necessary. Possibly two. The sources will be moved around, but there will be some non-repository python cache files left behind, that you can probably either ignore or remove with e.g.

find modules/dials -name __pycache__ | xargs rm -rf
find modules/dials -depth -type d -empty -delete
# or
cd dials && git clean -nxd # Replace the -n with -f if happy

@ndevenish
Copy link
Member Author

  • xfel_regression tests failed because it imported a single string from dials.stills_process, and in the src/ refactor dials.tests is no longer an actual installed module. For now, I've included just the portion of the tests that uses in the src/dials/tests subpackage in f26b953. This should probably be migrated.
  • There is also a failed test on my test machine for mpiexec not being available - I'm not sure what the CI tests do that makes this pass. I'll see if it runs in here, but don't imagine this is a blocker

@ndevenish
Copy link
Member Author

To aid migration, it now continues to install a blank libtbx.dials package with libtbx.pkg_utils.define_entry_points({}). This prevents double-registering of entry_points.

The other issue is lingering __pycache__/ folders in DIALS - which should not cause any problems. They can be removed with usage of git clean -nffdx or e.g.

find dials -type d -name __pycache__ | xargs rm -rf
find dials -depth -type d -empty -delete

@ndevenish
Copy link
Member Author

ndevenish commented Apr 19, 2022

So, because of the moved src/ layout, the xfel module will fail to compile if it's configured first in the module list, which it previously did because of the issue in cafaf4d. To manage this migration, I have:

  • Removed xfel from the DIALS "optional_dependencies" list. This means that it will always end up configured afterwards (because it's no longer a circular dependency), but at the cost that it now needs to be specified in the initial libtbx.configure - I think this is always the case?
  • Checked for this misconfiguration case and print an explicit error. If you run a libtbx.refresh and it's configured this way, it'll exit with this error:
    Error: xfel module is configured before dials, but now requires dials first.
    
    To fix this, please run:
    
            libtbx.configure --exclude=xfel $(libtbx.list_modules) xfel
    

I'm... not sure that there is a better way to manage this inside dials, without bothering the user. It's not easy to re-order these at time of refresh, because it's in a literal loop over the list, and changing that while running might be possible but would be a bit bodged (e.g. adding xfel a second time to the list...). But leaving it as-is means that if xfel is configured first, it will fail, because it doesn't know about the src/ layout and thus won't be able to find the headers.

(edit: Fiddling with the module list in the simple case won't work because the SConscripts would be called twice, causing potentially duplicate build instructions.)

@ndevenish
Copy link
Member Author

On the plus side, the xfel_regression CI tests appear to be passing now.

@ndevenish
Copy link
Member Author

And autocomplete works.

@ndevenish ndevenish marked this pull request as ready for review April 19, 2022 16:51
@ndevenish ndevenish changed the title WIP: Switch DIALS to src/ layout Switch DIALS to src/ layout Apr 19, 2022
Copy link
Member

@Anthchirp Anthchirp left a comment

Choose a reason for hiding this comment

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

Overall looking good.

I'm sure you've already thought of this, but: when merging this I'd be keen to squash it down to two commits, one just for the renames and one with changes that also adds the rename commit to that ignore file.

build.py Outdated Show resolved Hide resolved
libtbx_refresh.py Outdated Show resolved Hide resolved
Comment on lines +14 to +22
"long_description": Path(__file__).parent.joinpath("README.md").read_text(),
"description": "Diffraction Integration for Advanced Light Sources",
"author": "Diamond Light Source",
"license": "BSD-3-Clause",
"author_email": "dials-support@lists.sourceforge.net",
"project_urls": {
"homepage": "https://dials.github.io",
"repository": "https://github.com/dials/dials",
},
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest moving the bulk of the static information into a setup.cfg - happy to provide this as commit

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought I recalled us doing this for dxtbx, but it appears not to be the case? Did something not work?

@ndevenish

This comment was marked as outdated.

@ndevenish
Copy link
Member Author

I'm also happy to consider just "Run libtbx.configure automatically then exit" but this feels a little prone to just complicating things, and I don't know if it will break anything for custom environments.

@ndevenish
Copy link
Member Author

On Markus' suggestion I've reworded to be more explicit about what the DIALS change was:

image

@codecov
Copy link

codecov bot commented Apr 21, 2022

Codecov Report

Merging #2077 (c1bad1a) into main (80844a8) will increase coverage by 11.60%.
The diff coverage is 0.00%.

❗ Current head c1bad1a differs from pull request most recent head 0a27c21. Consider uploading reports for the commit 0a27c21 to get more accurate results

@@             Coverage Diff             @@
##             main    #2077       +/-   ##
===========================================
+ Coverage   68.48%   80.09%   +11.60%     
===========================================
  Files         654      576       -78     
  Lines       76497    65431    -11066     
  Branches    10903     9227     -1676     
===========================================
+ Hits        52392    52407       +15     
+ Misses      22062    10989    -11073     
+ Partials     2043     2035        -8     

@Anthchirp
Copy link
Member

If you get

Traceback (most recent call last):
  File "modules/cctbx_project/libtbx/env_config.py", line 3224, in <module>
    warm_start(sys.argv)
  File "modules/cctbx_project/libtbx/env_config.py", line 3129, in warm_start
    _warm_start(env, args)
  File "modules/cctbx_project/libtbx/env_config.py", line 3099, in _warm_start
    env.refresh()
  File "modules/cctbx_project/libtbx/env_config.py", line 2253, in refresh
    module.process_libtbx_refresh_py()
  File "modules/cctbx_project/libtbx/env_config.py", line 2570, in process_libtbx_refresh_py
    exec(to_str(fh.read()), global_vars)
  File "<string>", line 12, in <module>
ImportError: bad magic number in 'dials': b'\x03\xf3\r\n'
make: *** [reconf] Error 1

then you have to delete modules/dials/__init__.pyc.

With src/ layout, these are no longer inside the dials. package
xfel_regression directly imports from dials.tests to get a phil string out of
test_stills_process. For now, leave this in so that xfel_regression doesn't
break.
- Split setup.py into setup/build.py, like dxtbx. pip installing DIALS
  will now properly install all python sources into the correct
  site-packages location.
- Fix SConscript to build relatively, and update central environment
  configuration so that downstream packages know where the DIALS
  sources are.
- Historically, the xfel module could be configured before the dials
  module, because of a recursive dependency. Because dials now needs to
  update the source location variable that xfel uses, it now needs to be
  configured first. libtbx_refresh checks for this scenario, and aborts
  with an error and instructions on how to fix this.
- Fixup a few other references to now-src-relative locations
- Ignore the large rename commit in "git blame" views.
@ndevenish ndevenish deleted the src branch April 21, 2022 14:38
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

2 participants