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

Validate properties that refer to other datamodels #1008

Merged
merged 33 commits into from
Nov 28, 2024

Conversation

francescalb
Copy link
Collaborator

@francescalb francescalb commented Nov 20, 2024

This pull request includes changes to the dlite-validate file to improve the validation of datamodels that refer to other datamodels in their properties.

Question: Should we add error-handling here?

Problem that I do not know how to fix: If there is and error in the dimension in the references datamodel I get a segmentation fault and the whole process closes down without error message.

builds on #986
closes #1007

Type of change

  • Bug fix & code cleanup
  • New feature
  • Documentation update
  • Test update

Checklist for the reviewer

This checklist should be used as a help for the reviewer.

  • Is the change limited to one issue?
  • Does this PR close the issue?
  • Is the code easy to read and understand?
  • Do all new feature have an accompanying new test?
  • Has the documentation been updated as necessary?
    This pull request includes changes to the bindings/python/scripts/dlite-validate file to improve the validation process for instances and their properties. The most important changes include adding validation for property references and updating the import and spacing for better readability.

Copy link
Collaborator

@jesper-friis jesper-friis left a comment

Choose a reason for hiding this comment

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

Nice PR. Added some comments

@jesper-friis
Copy link
Collaborator

jesper-friis commented Nov 20, 2024

If you have time for it, it would be nice with some tests for this PR as well.

@jesper-friis
Copy link
Collaborator

Question: Should we add error-handling here?

Problem that I do not know how to fix: If there is and error in the dimension in the references datamodel I get a segmentation fault and the whole process closes down without error message.

First, I think that we should never get a segfault from Python. That is a bug that should be fixed independently. If there is an error in the dimensions an exception should be raised.

Regarding your question, dlite should hopefully raise an exception if there is an error. These are already handled, so I don't think we need to do additional error handling in dlite-validate.

However, if there is an error in the datamodel or instance that is not detected by dlite, we could either handle it here or update the underlying dlite framework to raise a proper exception. I think the choice depends on the nature of the error and its severity.

@francescalb
Copy link
Collaborator Author

francescalb commented Nov 20, 2024

If you have time for it, it would be nice with some tests for this PR as well.

In principle I agree, but the setup and the placements of tests and how to run them is a bit complicated. I have added some new valid and invalid datamodels that can be tested.

Note that the InvalidYaml2.yaml is the one that causes the segmentation fault. It happens when nothing is given the dimensions.

Base automatically changed from validate-multiple-instances to master November 21, 2024 09:06
@francescalb francescalb marked this pull request as ready for review November 22, 2024 08:21
@jesper-friis
Copy link
Collaborator

First, I think that we should never get a segfault from Python. That is a bug that should be fixed independently. If there is an error in the dimensions an exception should be raised.

PR #1010 should fix the segfault

@jesper-friis
Copy link
Collaborator

If you have time for it, it would be nice with some tests for this PR as well.

In principle I agree, but the setup and the placements of tests and how to run them is a bit complicated. I have added some new valid and invalid datamodels that can be tested.

See PR #1011 for how to add tests with CMake.

jesper-friis and others added 6 commits November 23, 2024 02:16
# Description
Avoid that dlite segfaults if "dimensions" is null (empty) in JSON and
YAML input.


## Type of change
- [x] Bug fix & code cleanup
- [ ] New feature
- [ ] Documentation update
- [ ] Test update

## Checklist for the reviewer
This checklist should be used as a help for the reviewer.

- [ ] Is the change limited to one issue?
- [ ] Does this PR close the issue?
- [ ] Is the code easy to read and understand?
- [ ] Do all new feature have an accompanying new test?
- [ ] Has the documentation been updated as necessary?
# Description
Added tests for dlite-validate of instances with ref-types.

Merge PR #1010 first to get rid of the segfaults.

## Type of change
- [ ] Bug fix & code cleanup
- [ ] New feature
- [ ] Documentation update
- [x] Test update

## Checklist for the reviewer
This checklist should be used as a help for the reviewer.

- [ ] Is the change limited to one issue?
- [ ] Does this PR close the issue?
- [ ] Is the code easy to read and understand?
- [ ] Do all new feature have an accompanying new test?
- [ ] Has the documentation been updated as necessary?
@francescalb francescalb merged commit 9a47df8 into master Nov 28, 2024
9 checks passed
@francescalb francescalb deleted the flb/validate_refs_properties branch November 28, 2024 09:06
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.

Ref not checked correctly in dlite-validate
2 participants