Skip to content

Conversation

@hattne
Copy link
Collaborator

@hattne hattne commented Jun 20, 2025

Runs the CMake build with nproc CPUs everywhere; this may not work for the Makefile build, on account of recursive invocations of make. Also, using --parallel with cmake in MSYS2 sinks the ship!

@hattne
Copy link
Collaborator Author

hattne commented Jun 20, 2025

This will run the Docker CMake build with nproc CPUs, which has previously been observed to excessively excercise the fans and slow down the build. Will throttling the Docker build by e.g. constraining the container to the first four CPUs (--cpuset-cpus=0-3) or only allowing it to consume half of the CPU capacity (--cpu-quota=50000) resolve this (see Resource constraints)?

If so, we could add something to that effect to .dev/docker/README.md; otherwise d97c2cb may have to be reverted.

@dwpaley Opinions?

hattne added 3 commits June 20, 2025 14:45
Obsoletes conda for the Python dependencies.  Matplotlib
(python3-matplotlib in Ubuntu) does not appear to be needed at the
moment.  Capitalize the GitHub Action step names for consistency.
Remove -v (--verbose) option from rsync and tar.  Do not store
intermediate downloads and elide seemingly redundant touch:es.  Note
that http://downloads.sf.net/cbflib/PyCifRW-4.3_rev_19Jun21.tar.gz
($(PY3CIFRWURL)) appears to be an uncompressed tarfile, despite its .gz
extension.
Use e.g. --cpuset-cpus or --cpu-quota to externally limit CPU usage with
docker.  The analogous make -j may not work for the Makefile build,
because make is run recursively.
hattne and others added 2 commits June 28, 2025 21:11
Affects the regeneration of e.g. the SWIG interface files.  This is
arguably a buildsystem bug.
@hattne hattne requested a review from dwpaley July 7, 2025 01:42
@hattne hattne marked this pull request as ready for review July 7, 2025 01:46
@dwpaley
Copy link
Collaborator

dwpaley commented Jul 7, 2025

Without objection here on the --parallel argument. On this branch, my laptop builds the cmake Dockerfile in a total of ~4 minutes: 1 for dependencies, 2 for the build, <1 to test.

It's funny that --parallel `nproc` works when simply --parallel doesn't, because how else is cmake deciding how many jobs to create? But it's not important.

About the source of the Python dependencies--I worry a bit because when installing thru APT we have less flexibility to choose versions. Currently APT on ubuntu 24:04 gives python 3.12.3 and Numpy 1.26.4; conda create -n test numpy gives python 3.13.5 and numpy 2.3.1. Of course the real answer would be to add explicit control of versions if we really care about it. In that event we might have to switch back to Conda, but this way is fine for now.

steps:
- uses: actions/checkout@v4
- name: install extra dependencies
- name: Install extra dependencies
Copy link
Collaborator

Choose a reason for hiding this comment

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

thank you, maybe my shift key is defective

@hattne
Copy link
Collaborator Author

hattne commented Jul 8, 2025

About the source of the Python dependencies--I worry a bit because when installing thru APT we have less flexibility to choose versions. Currently APT on ubuntu 24:04 gives python 3.12.3 and Numpy 1.26.4; conda create -n test numpy gives python 3.13.5 and numpy 2.3.1. Of course the real answer would be to add explicit control of versions if we really care about it. In that event we might have to switch back to Conda, but this way is fine for now.

Thanks, Dan! I'm not overly concerned about the different Python versions. The SWIG step, which I suppose is what the tests are supposed to exercise, doesn't really depend on Python. But the module that a Python interpreter builds from the SWIG output is specific to the version of the interpreter.

I think I see your point, though: if the SWIG input has bunch of version-conditionals there could be trouble! Nevertheless, given your approval, I'll merge this unless there are any objections within the next day or so.

@dwpaley
Copy link
Collaborator

dwpaley commented Jul 8, 2025

Definitely fine to merge. I was thinking of the version of numpy that the tests run on. The tests contain some lines like: d = numpy.frombuffer(s,numpy.uint32) which admittedly looks safe wrt numpy1/numpy2 issues. Confirming this in CI through a version matrix could be a future project.

@hattne hattne merged commit 16355cc into dials:main Jul 12, 2025
2 checks passed
@hattne hattne deleted the docker-make branch July 12, 2025 02:00
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