Skip to content

Conversation

@mhabedan
Copy link
Collaborator

@mhabedan mhabedan commented May 22, 2025

As a follow-up to HEPData/hepdata-converter#64, this PR adds yoda-H5 support for hepdata itself.
Namely the changes are

Tested locally on commit f923a755 from hepdata-converter.
Requires HEPData/hepdata-converter-ws#17.

@mhabedan
Copy link
Collaborator Author

Hi @GraemeWatt!
Now that HEPData/hepdata-converter-ws#17 is merged, this PR should be good to go, too! (It's the second to last one for yoda-h5, the last one being HEPData/hepdata-cli#7)

Copy link
Member

@GraemeWatt GraemeWatt left a comment

Choose a reason for hiding this comment

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

Thanks, looks good. I agree not to add the new yoda.h5 format to the dropdown menus initially, but it can easily be added later if there is demand for it. I checked out your branch and I confirm it works locally with the latest hepdata-converter-ws Docker image (see docs). I updated your branch to include the latest changes from the main branch. Few comments:

  1. The JSON format of a record contains links to various output formats of data tables. Probably the new yoda.h5 format should be added here.
  2. The yoda.h5 filename of a whole record is e.g. HEPData-ins1847779-v1-yoda.h5.tar.gz containing HEPData-ins1847779-v1-yoda.h5.yoda.h5. Is this OK or would it be better to change the . to a - if not in the file extension, e.g. HEPData-ins1847779-v1-yoda-h5.tar.gz containing HEPData-ins1847779-v1-yoda-h5.yoda.h5? This could be done by replacing file_format with file_format.replace('.', '-') here and here. Another option is e.g. HEPData-ins1847779-v1-yodah5.tar.gz containing HEPData-ins1847779-v1-yodah5.yoda.h5. Which do you prefer?
  3. I triggered the CI, but I think it's failing because your fork doesn't have access to the repository secrets, particularly OPENSEARCH_INITIAL_ADMIN_PASSWORD. The end-to-end tests also require SAUCE_ACCESS_KEY and SAUCE_USERNAME. We could change the GitHub Actions workflow to use a pull_request_target event, but I'd need to investigate how this works and if it is secure. I've invited you as a Collaborator with Write access to the main HEPData/hepdata repository, so in future you can create a new branch under this repository. I've sent you the necessary secrets privately that you can add to your fork to avoid creating a new branch and opening a new PR.

@mhabedan
Copy link
Collaborator Author

mhabedan commented Aug 6, 2025

Hi @GraemeWatt!

Thanks for your review and apologies for being so slow for acting on it!

  1. Done.
  2. That's a good point, the previous notation was confusing. I opted for HEPData-ins1847779-v1-yoda-h5.tar.gz containing HEPData-ins1847779-v1-yoda-h5.yoda.h5.
  3. I've added the secrets but the CI still fails because the connection was refused. Will follow-up via email.

@mhabedan mhabedan mentioned this pull request Aug 14, 2025
@GraemeWatt GraemeWatt merged commit 2ce7ef2 into HEPData:main Aug 19, 2025
7 of 13 checks passed
@GraemeWatt
Copy link
Member

I'm not sure why this PR is shown as merged after I merged the replacement PR #899, but the commits to the main branch look correct. I edited the CONTRIBUTING.rst file in commit e42a7e7 to request that future PRs should not be made from forks.

@mhabedan mhabedan deleted the yodaH5 branch September 22, 2025 16:02
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