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

Metadata code refactoring #679

Merged
merged 48 commits into from
Nov 21, 2023
Merged

Metadata code refactoring #679

merged 48 commits into from
Nov 21, 2023

Conversation

GernotMaier
Copy link
Contributor

@GernotMaier GernotMaier commented Nov 15, 2023

(sorry, this is a difficult one)

This is a major pull request with hopefully improvements on how to handle the metatdata required for each input to a simtool and generate for each output of a simtool. It simplifies the metatdata treatment (note: more lines deleted than created), but unfortunately metadata collection is still quite fine tuned.

The problem with the current implementation is outlined in issue #678 .

Changes:

  • use jsonschema to define metadata
  • encoded in simtools/schemas/metadata.schema.yml:
    • allows for a full description of all fields (including examples, allowed values, types, etc)
  • new application: simtools-generate-default-metadata-file to generate an example file from default values
  • allows to use the application simtools-validate-schema-files to validate metadata
    • good for testing: metadata generated with simtools-generate-default-metadata-file must pass simtools-validate-schema-files
    • renamed this application to simtools-validate-file-using-schema (which is clearer, as we don't validate a schema but use a schema to validate a file)
  • using the jsonschema package allows to remove simtools/data_model/validate_schema.py
  • changes to pip packaging so that simtools/schemas/*.yml is included in the package
  • (a surprising amount of language typos fixed)
  • updated all test files in tests/resources/meta (the pass now simtools/applications/validate_file_using_schema.py

@GernotMaier GernotMaier self-assigned this Nov 15, 2023
@GernotMaier GernotMaier linked an issue Nov 15, 2023 that may be closed by this pull request
Copy link

codecov bot commented Nov 16, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c779140) 99.30% compared to head (bf0e8c7) 99.31%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #679      +/-   ##
==========================================
+ Coverage   99.30%   99.31%   +0.01%     
==========================================
  Files          40       39       -1     
  Lines        3575     3485      -90     
==========================================
- Hits         3550     3461      -89     
+ Misses         25       24       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@GernotMaier GernotMaier marked this pull request as ready for review November 16, 2023 09:34
GernotMaier and others added 6 commits November 16, 2023 17:28
Copy link
Contributor

@VictorBarbosaMartins VictorBarbosaMartins left a comment

Choose a reason for hiding this comment

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

Uff, this was a long review. I know this is a one-time PR with a very nice update on the metadata handling, but let's try to keep PRs small. The applications, for example, could have waited for the next PR.

In any case, thanks a lot for these updates, as we have spoke early today, this is a major improvement and therefore I also dedicated more time to the review. I provide some comments below. The main issue I raise is in regard to the collection of functions in metadata_model and the lack of an appropriate class there. We can further discuss it on Wednesday if you prefer.

Please note that I did not double check all entries in the metadata file (should I?). I also did not look very deeply into the unit tests to check whether all functionalities are being tested, though I expect them to.

Thanks!

docs/source/applications.rst Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
simtools/applications/generate_default_metadata_file.py Outdated Show resolved Hide resolved
simtools/applications/generate_default_metadata_file.py Outdated Show resolved Hide resolved
simtools/applications/generate_default_metadata_file.py Outdated Show resolved Hide resolved
simtools/data_model/metadata_model.py Outdated Show resolved Hide resolved
simtools/schemas/metadata.schema.yml Show resolved Hide resolved
simtools/schemas/metadata.schema.yml Show resolved Hide resolved
@GernotMaier
Copy link
Contributor Author

Thanks for the review!

The application where actually essential for this pull requests, as they allowed me to test the new implementation.

Everything else is fine and I am going to address your comments now.

Uff, this was a long review. I know this is a one-time PR with a very nice update on the metadata handling, but let's try to keep PRs small. The applications, for example, could have waited for the next PR.

In any case, thanks a lot for these updates, as we have spoke early today, this is a major improvement and therefore I also dedicated more time to the review. I provide some comments below. The main issue I raise is in regard to the collection of functions in metadata_model and the lack of an appropriate class there. We can further discuss it on Wednesday if you prefer.

Please note that I did not double check all entries in the metadata file (should I?). I also did not look very deeply into the unit tests to check whether all functionalities are being tested, though I expect them to.

Thanks!

@GernotMaier
Copy link
Contributor Author

@VictorBarbosaMartins - thanks again for the review, a couple of good points which improved the code.

I think I have addressed all your points, let me know if not.

VictorBarbosaMartins and others added 2 commits November 21, 2023 10:07
Signed-off-by: Victor Barbosa Martins <[email protected]>
@VictorBarbosaMartins
Copy link
Contributor

a50b9a5

Hi Gernot. Thank you for considering the comments. I hope they help to improve the code. I left some open items but in general it is already very good! Let me know when you are done with the new changes and I will approve.

@GernotMaier
Copy link
Contributor Author

@VictorBarbosaMartins - thanks for the detailed review. I think everything is addressed now. Let me know if I can merge (after the tests passed).

Copy link
Contributor

@VictorBarbosaMartins VictorBarbosaMartins left a comment

Choose a reason for hiding this comment

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

I am fine with the changes, so I am approving. Thanks for the implementation and for taking my comments into consideration. It was really helpful to sit together before the review, let's try to do that more often for more complicated PRs.

@GernotMaier GernotMaier merged commit 40ed0d8 into main Nov 21, 2023
11 checks passed
@GernotMaier GernotMaier deleted the metadata_refactoring branch November 21, 2023 10:29
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.

Refactoring metadata handling
2 participants