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

Unpin lalsuite version #5040

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

spxiwh
Copy link
Contributor

@spxiwh spxiwh commented Feb 14, 2025

The various data files needed for lalsuite are now available publically again (lalsuite will inform the user of this if the user does not have the files in their path).

Therefore I unpin lalsuite. I've dumped the new SEOBNR file into our existing CVMFS directory so the CI will need no more changes.

To confirm SEOBNRv4_ROM_v3 is identical to SEOBNRv4_ROM_v2 (it just contains a certain bit of metadata that is irrelevant for us).

This would be needed for #5019 (and a lalsuite release that uses igwn-ligolw, if that doesn't already exist).

@titodalcanton
Copy link
Contributor

Don't you also need to change something here? https://github.com/gwastro/pycbc/blob/master/.github/workflows/basic-tests.yml#L31-L37

@titodalcanton
Copy link
Contributor

Huh, HDF5 errors and a segfault, interesting…

@spxiwh
Copy link
Contributor Author

spxiwh commented Feb 14, 2025

Huh, HDF5 errors and a segfault, interesting…

I think they're the same error, and I think it originates with me not understanding git lfs ... Grumble, grumble, grumble ... Moving to a direct curl instead, I understand curl.

@titodalcanton
Copy link
Contributor

But the segfault is indicative of a bigger problem, potentially an unchecked error condition in lalsimulation.

@spxiwh
Copy link
Contributor Author

spxiwh commented Feb 14, 2025

But the segfault is indicative of a bigger problem, potentially an unchecked error condition in lalsimulation.

That's probably right .... But I'm not sure I want to go down that rabbit hole. Jolien's HDF code did fail, so I suspect that SEOB carried on with a NULL pointer, which will lead to predictable outcomes. Writing [XLAL] C-code that's proof against every possible fail mode is a massive PITA

@@ -35,6 +35,7 @@ jobs:
git lfs pull -I "data/lalsimulation/SEOBNRv4ROM_v2.0.hdf5"
mv data/lalsimulation/* ../
cd ../
curl https://git.ligo.org/waveforms/software/lalsuite-waveform-data/-/raw/main/waveform_data/SEOBNRv4ROM_v3.0.hdf5 > SEOBNRv4ROM_v3.0.hdf5
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
curl https://git.ligo.org/waveforms/software/lalsuite-waveform-data/-/raw/main/waveform_data/SEOBNRv4ROM_v3.0.hdf5 > SEOBNRv4ROM_v3.0.hdf5
curl --show-error --silent --remote-name https://git.ligo.org/waveforms/software/lalsuite-waveform-data/-/raw/main/waveform_data/SEOBNRv4ROM_v3.0.hdf5

Copy link
Contributor

@titodalcanton titodalcanton Feb 14, 2025

Choose a reason for hiding this comment

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

In fact, we may as well just use curl for the files above and get rid of all git/cd/mv commands, including the installation of git-lfs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SEOBNR_v2_ROM is like 20 separate files, which might be messy. But let me see if just getting the v4 files is sufficient now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Drop the > redirects, that is the point of --remote-name.

Copy link
Contributor

@titodalcanton titodalcanton left a comment

Choose a reason for hiding this comment

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

I have a minor nitpick above, but this looks ok.

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