Skip to content

CS connection refurbishment#88

Closed
JeanLucPons wants to merge 67 commits intomainfrom
cs-refurbishment-multiple-csprefix
Closed

CS connection refurbishment#88
JeanLucPons wants to merge 67 commits intomainfrom
cs-refurbishment-multiple-csprefix

Conversation

@JeanLucPons
Copy link
Copy Markdown
Contributor

@JeanLucPons JeanLucPons commented Dec 4, 2025

The aim of this PR is:

  • Allow multiple control system to be used.
  • Remove the int_cs() call.
  • Provides a better symmetry between lattices and control systems.

It shows minimum modifications to handle the above and change the way to handle external magnet model and CS backend. This new branch is not compatible with the present Tango backend.
If this PR is accepted then I'll port the Tango backend.

If we decide to limit pyAML to a single control system per Accelerator then this PR is not mandatry and we can change the controls field to control and fix the name to live.

control:
    type: tango.pyaml.controlsystem
    tango_host: ebs-simu-3:10000

@JeanLucPons JeanLucPons marked this pull request as draft December 4, 2025 10:34
@JeanLucPons JeanLucPons changed the title CS connection refurbishment WIP: CS connection refurbishment Dec 4, 2025
@TeresiaOlsson
Copy link
Copy Markdown
Contributor

About the part:

"If we decide to limit pyAML to a single control system per Accelerator then this PR is not mandatry and we can change the controls field to control and fix the name to live."

Wouldn't that mean that you no longer can configure the virtual accelerator? For me that would be a second controls with a different host/PV prefix?

@JeanLucPons
Copy link
Copy Markdown
Contributor Author

Wouldn't that mean that you no longer can configure the virtual accelerator? For me that would be a second controls with a different host/PV prefix?

If we limit to one single CS per accelerator, it will be possible to load a second accelerator. So instead of writing:

sr.live...
sr.shadow...

You will write:

sr.live...
shadow.live...

…rbit-correction-and-orm

Orbit correction and Orbit Response Matrix measurement through pySC.
@TeresiaOlsson
Copy link
Copy Markdown
Contributor

Wouldn't that mean that you no longer can configure the virtual accelerator? For me that would be a second controls with a different host/PV prefix?

If we limit to one single CS per accelerator, it will be possible to load a second accelerator. So instead of writing:

sr.live...
sr.shadow...

You will write:

sr.live...
shadow.live...

Hmm... I think that will be confusing and potentially unsafe since then it's no longer given that live always will be the live machine. We have that setup in MML at the moment if we run it against the twin because then MML requires it to be run in online mode. I don't like it because then you always feel nervous about if online really means twin or you by accident loaded the real machine.

Also, what would that mean for the simulators? There would be one set of simulator objects for the live and another one for the shadow? Feels like that also potentially could cause problems.

@JeanLucPons
Copy link
Copy Markdown
Contributor Author

JeanLucPons commented Dec 11, 2025

It is now possible to write and run it without any backend installed:

sr = Accelerator.load("tests/config/EBSOrbit.yaml",ignore_external=True)
print(sr.design.get_magnets("VCorr").strengths.get())
        ignore_external: bool
            Ignore external modules and return None for object that
            cannot be created. pydantic schema that support that an
            object is not created should handle None fields.

@TeresiaOlsson , @gubaidulinvadim is it ok for you ?

@JeanLucPons JeanLucPons marked this pull request as ready for review December 11, 2025 16:19
@JeanLucPons JeanLucPons dismissed gubaidulinvadim’s stale review December 11, 2025 16:21

Array of None are necessary

@TeresiaOlsson
Copy link
Copy Markdown
Contributor

It's okay for me. I'm also working on something for the factory for issue #28 which might remove the need for this but I'm not sure yet and it will likely take a while until I manage to finish my suggestion...

@JeanLucPons JeanLucPons requested a review from kparasch December 11, 2025 17:42
@JeanLucPons
Copy link
Copy Markdown
Contributor Author

JeanLucPons commented Dec 11, 2025

It would be nice that this PR is rapidly merged as it has a strong probability of merge conflict.
I will modify the Tango and EPICS backend once it is merged.

@TeresiaOlsson
Copy link
Copy Markdown
Contributor

TeresiaOlsson commented Dec 11, 2025

It would be nice that this PR is rapidly merged as it has a strong probability of merge conflict. I will modify the Tango and EPICS backend once it is merged.

I agree since what you are doing in the load in the Accelerator looks similar to what I'm trying to do. I will try to go through the whole thing tomorrow.

Copy link
Copy Markdown
Contributor

@gubaidulinvadim gubaidulinvadim left a comment

Choose a reason for hiding this comment

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

Approved. Strangely, GitHub shows to me in the diff that tuning_tools/dispersion and some others is added in this PR when it should already be on the main branch.

@JeanLucPons JeanLucPons dismissed gubaidulinvadim’s stale review December 11, 2025 22:10

The merge-base changed after approval.

@gubaidulinvadim
Copy link
Copy Markdown
Contributor

We should also add a minor version bump as it will break compatibility with tango/ophyd-async bindings

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.

Add option to disable the control system when loading a PyAML file

4 participants