-
Notifications
You must be signed in to change notification settings - Fork 14
Add support for yoda-H5 #884
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
Conversation
|
Hi @GraemeWatt! |
There was a problem hiding this 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:
- The JSON format of a record contains links to various output formats of data tables. Probably the new
yoda.h5format should be added here. - The
yoda.h5filename of a whole record is e.g.HEPData-ins1847779-v1-yoda.h5.tar.gzcontainingHEPData-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.gzcontainingHEPData-ins1847779-v1-yoda-h5.yoda.h5? This could be done by replacingfile_formatwithfile_format.replace('.', '-')here and here. Another option is e.g.HEPData-ins1847779-v1-yodah5.tar.gzcontainingHEPData-ins1847779-v1-yodah5.yoda.h5. Which do you prefer? - 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 requireSAUCE_ACCESS_KEYandSAUCE_USERNAME. We could change the GitHub Actions workflow to use apull_request_targetevent, but I'd need to investigate how this works and if it is secure. I've invited you as a Collaborator withWriteaccess 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.
|
Hi @GraemeWatt! Thanks for your review and apologies for being so slow for acting on it!
|
As a follow-up to HEPData/hepdata-converter#64, this PR adds yoda-H5 support for hepdata itself.
Namely the changes are
.yoda.h5in supported file formats for download requests/urls..yoda.h5..yoda.h5to the dropdown menus.Tested locally on commit f923a755 from hepdata-converter.
Requires HEPData/hepdata-converter-ws#17.