Skip to content

Conversation

@EZoni
Copy link
Member

@EZoni EZoni commented Dec 17, 2025

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:

  • Clone the examples/ directory from the main PALS repository.
  • Loop over all *.pals.yaml files in the examples/ directory and read, parse, and validate the data from each file.

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":

Screenshot from 2026-01-06 13-52-04

To do

  • Fix the Python implementation and/or the examples in the main PALS repository.
  • Add as separate CI workflow (instead of separate step in the existing workflow) and mark as not required?

@EZoni EZoni added the CI/CD label Dec 17, 2025
@EZoni EZoni force-pushed the run_examples_from_pals branch from c78a626 to 085fc2c Compare December 17, 2025 18:12
@EZoni
Copy link
Member Author

EZoni commented Dec 17, 2025

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 list

I 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: 3

Possibly 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.

@EZoni EZoni requested review from ax3l and cemitch99 December 17, 2025 18:20
@EZoni
Copy link
Member Author

EZoni commented Dec 20, 2025

I think if we call BeamLine(data) instead of BeamLine(**data) (this is wrong for a Python list regardless...) and add a custom __init__ similar to the following one (vibe-coded) to the BeamLine class,

+    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
Copy link
Member

@ax3l ax3l Jan 7, 2026

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

@ax3l
Copy link
Member

ax3l commented Jan 7, 2026

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.

@EZoni

This comment was marked as outdated.

args = parser.parse_args()
example_file = args.path
# Parse and validate YAML data from file
BeamLine.from_file(example_file)
Copy link
Member Author

Choose a reason for hiding this comment

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

@ax3l

Given the latest changes to the FODO example in the main PALS repository, https://github.com/pals-project/pals/blob/main/examples/fodo.pals.yaml, how did you envision parsing the data from file now?

I'm trying to understand what we're missing:

  • A Lattice class?
  • New methods to read from file or dump to file, not from the BeamLine class?
  • ...

Copy link
Member

@ax3l ax3l Feb 10, 2026

Choose a reason for hiding this comment

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

Thanks! Yes I think we should add a Lattice class next! The current methods funnel all into BeamLine but we need to go one level up to Lattice and users would interact with that class now.

(If a file has multiple lattices, we could sub-select one as an optional argument to from_file, but this can be done later.)

Copy link
Member Author

@EZoni EZoni Feb 10, 2026

Choose a reason for hiding this comment

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

And what about the facility and PALS extra levels? You want the Lattice class to understand those too?

This is an example of the dictionary structure that is read from file:

{'PALS': {'version': None, 'facility': [{'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': 3}}]}}, {'fodo_lattice': {'kind': 'Lattice', 'branches': ['fodo_channel']}}, {'use': 'fodo_lattice'}]}}

Copy link
Member Author

@EZoni EZoni Feb 10, 2026

Choose a reason for hiding this comment

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

Besides how to handle the PALS and facility layers, we also need to plan a design to handle inheritance (direct or indirect) and repetition.

- Copy implementation from BeamLine to Lattice
- Move methods from_file, to_file from BeamLine to Lattice
- Use new class in external examples test script

kind: Literal["Lattice"] = "Lattice"

line: List[get_all_elements_as_annotation()]
Copy link
Member Author

Choose a reason for hiding this comment

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

Based on pals/examples/fodo.pals.yaml#L46-L49

    - fodo_lattice:
        kind: Lattice
        branches:
          - fodo_channel

this should be branches:

Suggested change
line: List[get_all_elements_as_annotation()]
branches: List[get_all_elements_as_annotation()]

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, should this be Annotated[Union[BeamLine], Field(discriminator="kind")] instead of get_all_elements_as_annotation()?

Suggested change
line: List[get_all_elements_as_annotation()]
line: List[Annotated[Union[BeamLine], Field(discriminator="kind")]]

I don't remember if a Lattice is supposed to be built only on BeamLines or if it can be built on any other element kind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants