Skip to content

Analysis schema#906

Merged
GraemeWatt merged 35 commits into
mainfrom
analysisSchema
Sep 8, 2025
Merged

Analysis schema#906
GraemeWatt merged 35 commits into
mainfrom
analysisSchema

Conversation

@mhabedan
Copy link
Copy Markdown
Collaborator

@mhabedan mhabedan commented Sep 2, 2025

Following up on a discussion of the OpenMAPP project and #878, this PR adds a JSON schema to HEPData that defines the format of the input "analyses" JSONs.

This new PR is not based on a fork so all test should run successfully.

Furthermore, this PR now adds the last two items discussed in #878:

  • A JSON schema for the previously used standard (called "0.1.0") is added.
  • The toggle between running the old or new processing of analyses JSONs is now based on the existence and version of the schema_version field.

@coveralls
Copy link
Copy Markdown

coveralls commented Sep 2, 2025

Coverage Status

coverage: 84.421% (+0.3%) from 84.073%
when pulling 3897cff on analysisSchema
into 55a450b on main.

@GraemeWatt GraemeWatt mentioned this pull request Sep 2, 2025
Copy link
Copy Markdown
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 for opening the new PR. Tests are passing, but test coverage decreased by 0.6%, mainly because the new code added to the update_analyses function is not being tested. I see two options (see also my previous comment):

  1. Move the changes to the update_analyses function to (a new version of) PR #886 as you had originally, where you added tests using GAMBIT.
  2. Move the remaining commits of PR #886 into this PR (i.e. the GAMBIT support and tests) to increase the test coverage.

Comment thread hepdata/modules/records/utils/analyses.py Outdated
Comment thread hepdata/modules/records/utils/analyses.py Outdated
Comment thread hepdata/modules/records/utils/analyses.py Outdated
Comment thread hepdata/templates/analyses_schema/1.0.0/analyses_schema.json Outdated
@mhabedan
Copy link
Copy Markdown
Collaborator Author

mhabedan commented Sep 4, 2025

Thanks for the review, @GraemeWatt! I've addressed all issues. Regarding the decreased test coverage, I opted for using SModelS as test case as it is currently the most mature option. All tests should come back as green as soon as the issue you raised in #878 (comment) is addressed.

* Simplify test_general_pages to avoid calling flask.url_for twice.
* Access URLs for submission_schema, analyses_schema, formats, ping.
* Test case of updating SModelS analyses with no analyses to add.
* Test case of updating analyses for an endpoint with no endpoint_url.
@GraemeWatt GraemeWatt requested a review from Copilot September 5, 2025 10:16
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a JSON schema framework for defining how analysis implementation tools communicate with HEPData. It adds support for both a new structured schema (version 1.0.0) and backward compatibility with the previous format (version 0.1.0), using the presence and value of the schema_version field to determine which processing approach to use.

  • Adds JSON schema definitions and documentation for both v1.0.0 and v0.1.0 analysis formats
  • Updates analysis processing logic to handle both old and new schema formats based on schema_version field
  • Adds comprehensive test coverage for the new schema validation functionality

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
tests/test_data/analyses_example.json Example file demonstrating the new v1.0.0 schema format
tests/records_test.py Updated test expectations to match new schema processing behavior
tests/e2e/test_general.py Added end-to-end tests for new schema endpoints and refactored existing tests
tests/analyses_schema_test.py New test file specifically for validating the analysis schema
hepdata/version.py Minor version bump to reflect changes
hepdata/templates/analyses_schema/ Added schema definitions and documentation for both v0.1.0 and v1.0.0
hepdata/modules/theme/views.py New endpoint for serving analysis schema files
hepdata/modules/records/utils/analyses.py Core logic updates to support dual schema processing
hepdata/config.py Updated SModelS endpoint configuration

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread hepdata/modules/records/utils/analyses.py Outdated
Comment thread hepdata/modules/records/utils/analyses.py Outdated
Comment thread hepdata/modules/records/utils/analyses.py Outdated
Comment thread hepdata/modules/records/utils/analyses.py Outdated
Comment thread hepdata/modules/records/utils/analyses.py Outdated
Comment thread hepdata/modules/records/utils/analyses.py Outdated
@GraemeWatt
Copy link
Copy Markdown
Member

@codecov-ai-reviewer review

@ghost

This comment has been minimized.

@ghost

This comment has been minimized.

@GraemeWatt
Copy link
Copy Markdown
Member

@codecov-ai-reviewer review

@ghost

This comment has been minimized.

Comment thread hepdata/modules/records/utils/analyses.py
Comment thread hepdata/modules/records/utils/analyses.py
Comment thread hepdata/config.py
Comment thread hepdata/modules/theme/views.py
mhabedan and others added 4 commits September 5, 2025 11:37
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Clarify some code comments and use spaces around comparison operators.
* Catch ValidationError exception and add a test for coverage.
* Also replace "is not" by "!=" twice in records_test.py.
* submission_schema, analyses_schema and ping in test_general_pages
  return either JSON or "OK" where favicon.ico is not present.
Copy link
Copy Markdown
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.

@mhabedan : I think this is ready to be merged now. Please make a final check. Thanks for all your work on this PR.

My only concern is that the analyses_schema.json and readme.md files are buried inside the templates folder (which is meant for HTML templates), so they are not easily accessible, but I can link them from the "Analyses" section of the HEPData submission documentation.

@mhabedan
Copy link
Copy Markdown
Collaborator Author

mhabedan commented Sep 8, 2025

Hi @GraemeWatt!

Had another read-through and fixed a few minor typos in the readme/ JSON schema comments. It's good to go now in my opinion.

Regarding the findability: Yes, it would certainly be good if it was easier to find. Linking in the "Analysis" section sounds good to me. We can also link it from the OpenMAPP webpage. Not sure if we can do anything apart from that though.

@GraemeWatt GraemeWatt merged commit bd395c7 into main Sep 8, 2025
7 checks passed
@GraemeWatt GraemeWatt deleted the analysisSchema branch September 8, 2025 15:56
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.

4 participants