-
Notifications
You must be signed in to change notification settings - Fork 3
[WIP] Add test to validate examples in the main PALS repository #55
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
base: main
Are you sure you want to change the base?
Conversation
| # Copy examples directory from the main PALS repository | ||
| cd examples | ||
| git clone --no-checkout https://github.com/pals-project/pals.git pals_temp | ||
| cd pals_temp | ||
| git sparse-checkout init | ||
| git sparse-checkout set examples/ | ||
| git checkout main |
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.
Sparse checkout here to avoid cloning the entire repository.
c78a626 to
085fc2c
Compare
| data = yaml.safe_load(file) | ||
| # Parse and validate YAML data | ||
| print(f"Parsing data from {example_file}...") | ||
| BeamLine(**data) |
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.
Loading the data into a BeamLine object triggers automatic validation.
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.
Also note that this, BeamLine(**data), is how we currently load data in our unit tests, e.g.,
pals-python/tests/test_serialization.py
Lines 25 to 26 in 6998a31
| # Parse the YAML data back into a BeamLine object | |
| loaded_line = pals.BeamLine(**yaml_data) |
which is why I would expect it to work for the examples in the main PALS repository as well.
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.
Correct, this is the Python syntax for dictionary unpacking. YAML/JSON/... loads return a dictionary, and the BeamLine constructor expects parameters name, line, etc. that are the key-value pairs in the serialized dicts.
This is clean, Pythonic and needs no change.
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.
I added #57, which we can use here, too.
|
The test shows indeed that the Python implementation and the examples in the main PALS repository are not consistent (which, in turn, shows that this test is indeed useful): Traceback (most recent call last):
Parsing data from pals_temp/examples/fodo.pals.yaml...
File "/home/runner/work/pals-python/pals-python/examples/test_external_examples.py", line 26, in <module>
main()
File "/home/runner/work/pals-python/pals-python/examples/test_external_examples.py", line 22, in main
BeamLine(**data)
TypeError: pals.kinds.BeamLine.BeamLine() argument after ** must be a mapping, not listI think the Python implementation does not like that the example in the main PALS repository is structured as a list (each item starting with - drift1:
kind: Drift
length: 0.25
- quad1:
kind: Quadrupole
MagneticMultipoleP:
Bn1: 1.0
length: 1.0
- fodo_cell:
kind: BeamLine
line:
- drift1
- quad1
- drift2:
kind: Drift
length: 0.5
- quad2:
inherit: quad1
MagneticMultipoleP:
Bn1: -1.0
- drift1
- fodo_channel:
kind: BeamLine
line:
- fodo_cell:
repeat: 3Possibly related to #32? Should we fix the Python implementation or the syntax in the main repository in this case? P.S. Note that once this is fixed, the test will still fail because the Python implementation does not support inheritance yet. |
|
I think if we call + def __init__(self, data=None, **kwargs):
+ """Custom init to accept data as a positional argument"""
+ if data is not None and not kwargs:
+ # If data is passed as a positional argument without kwargs,
+ # use model_validate to process it through validators
+ instance = self.__class__.model_validate(data)
+ # Copy the validated attributes to this instance
+ for key, value in instance.__dict__.items():
+ object.__setattr__(self, key, value)
+ if hasattr(instance, '__pydantic_fields_set__'):
+ object.__setattr__(self, '__pydantic_fields_set__', instance.__pydantic_fields_set__)
+ else:
+ # Otherwise use the normal Pydantic initialization
+ super().__init__(**kwargs)
+the test does pass on a simpler FODO example (without the features that haven't been implemented yet), where the data is structured as a list. However, I'm not sure if this is the right way to go. I think we need to make a decision at the API level first. |
| run: | | ||
| # Copy examples directory from the main PALS repository | ||
| cd examples | ||
| git clone --no-checkout https://github.com/pals-project/pals.git pals_temp |
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.
For stability (avoid triggering limits), we want to use the GH action checkout for all checkouts.
It supports sub-options for creating the clone in a specific folder. It also supports sparse checkouts and shallow fetch depths.
https://github.com/actions/checkout?tab=readme-ov-file#checkout-v6
|
Thanks for starting this. We had a few more changes pending for the main document that we have not acted yet on (was discussed in Aug 2025). I added a PR in pals-project/pals#152 and once we have reviewed and merged this we could finalize here. |
Overview
This PR adds to the main CI workflow a new step that tests the examples in the main PALS repository.
The test goes as follows:
This makes sure that the Python implementation is actually consistent with the examples in the main PALS repository.
The examples within the PALS Python repository are labeled "internal", while the ones within the main PALS repository are labeld "external":
To do