Add in support for pulling out extra params fields defined in instrum… #57

jnation3406 · 2025-10-02T05:13:54Z

…ent_type validation_schema for OCS

This will add support for BLANCO, and also adds the "defocus" extra param validation for all LCO instruments. This could by extended in the future to add in rules for extra_params of other models like Targets, Constraints, etc., (slightly more difficult) and also to support rules on non-extra params fields imposed by the validation schema (much more difficult), and also to support rules imposed by validation schemas of specific modes (the most difficult). But just this PR is enough for BLANCO to work with its new fields.

…ent_type validation_schema for OCS

Fingel

Thanks Jon!

Crazy work on the jinja template there, that must have hurt your eyes.

While this does technically work, I was hoping for something more generic, as you alluded to in your commit message this only works for a subset of fields, and it gets more difficult for the others.

My view of this is that leaving extra_fields as a dict[Any, Any] is exactly how people are already using the API now. For example, if someone wants to use BLANCO, they have to somewhere (I don't even know where) look up the values for extra_fields and place them in their JSON request. The OCS API docs will not help them here. This is a fundamental downside to allowing your API to accept arbitrary data like that. So what Aeonlib is doing now isn't worse than the current API, it's on par with it.

So in my opinion, if we are going to try and work around arbitrary fields by parsing cerberus schemas, we should try to do it in the best possible way, otherwise what are we really gaining? A bunch of hardcoded tech debt for a small subset of fields.

I think we should do it the hard way.

Fingel · 2025-10-02T16:29:34Z

src/aeonlib/ocs/blanco/instruments.py

 # This file is generated automatically and should not be edited by hand.

-from typing import Any, Annotated, Literal
+from typing import Any, Annotated, Literal, Optional


I can see this being a pain because this is generated code, and if there doesn't happen to be any optional attributes then this warning doesn't make sense.

Luckily, using Optional is discouraged. The correct way to annotate optional types now is to use Type | None like defocus: float | None = None

That's good to know, I was figuring out pydantic and jinja2 as i went...

Fingel · 2025-10-02T16:31:24Z

codegen/lco/templates/instruments.jinja

+    model_config = ConfigDict(validate_assignment=True, extra='allow')
+    {% for field, properties in ctx.instrument_config_extra_params.items() %}
+    {% set optional = ('required' not in properties or not properties.required) and 'default' not in properties %}
+    {{ field }}: {% if optional %}Optional[{% endif %}{% if 'allowed' in properties %}Literal[{{ properties.allowed }}]{% else %}Annotated[{% if properties.type == 'string' %}str{% elif properties.type == 'integer' %}int{% elif properties.type == 'float' %}float{% elif properties.type == 'boolean' %}bool{% endif %}{% if 'min' in properties %}, Ge({{ properties.min }}){% endif %}{% if 'max' in properties %}, Le({{ properties.max }}){% endif %}]{% endif %}{% if optional %}] = None{% elif 'default' in properties %} = {{ properties | extract_default }}{% endif %}


Is this 500 column line the same as the 500 column line above? It's near impossible to read. Is there no way to factor this out? I was hoping this could be done in Python, like using a string builder pattern or something? Now you see why I was reaching for a parser...

Yes they are exactly the same - in fact the configuration and instrument configuration extra params sections are exactly the same except what they are iterating over. I don't know enough about jinja2 to figure out if you can wrap this in a template function or something to reuse. I thought a lot of this had to be done in the template and not in a python function because its using classes and stuff - the same reason you are using a jinja2 template at all to generate this?

Fingel · 2025-10-02T16:33:43Z

codegen/lco/generator.py

                    for k, v in ins["optical_elements"].items()
                },
+                "configuration_extra_params": ins['validation_schema'].get('extra_params', {}).get('schema', {}),
+                "instrument_config_extra_params": ins['validation_schema'].get('instrument_configs', {}).get('schema', {}).get('schema', {}).get('extra_params', {}).get('schema', {})


I really don't like how brittle this is. I know you think I was over-engineering it, but I was hoping for a more generic ability to iterate over fields and check for the existence of validate_schema for an object, not just hardcoding the json path for a specific instance of it. But maybe this is how it has to be done.

The validation_schema on the instrument_type is applied at the configuration level of our request language. What you call brittle here is just me either returning the proper extra_params fields added for configuration or instrument_configuration (the most common ones used), or returning {} if none exists in the validation_schema. I'm not sure how that is brittle?

Fingel · 2025-10-02T16:34:09Z

src/aeonlib/ocs/blanco/instruments.py

+class BlancoNewfirmConfigExtraParams(BaseModel):
+    model_config = ConfigDict(validate_assignment=True, extra='allow')
+    dither_value: Annotated[int, Ge(0), Le(1600)] = 80
+    dither_sequence: Literal[['2x2', '3x3', '4x4', '5-point']] = "2x2"


Invalid syntax, There's an extra set of [] in there.

oops, ill remove that

Fingel · 2025-10-02T16:35:44Z

src/aeonlib/ocs/blanco/instruments.py

+    dither_value: Annotated[int, Ge(0), Le(1600)] = 80
+    dither_sequence: Literal[['2x2', '3x3', '4x4', '5-point']] = "2x2"
+    detector_centering: Literal[['none', 'det_1', 'det_2', 'det_3', 'det_4']] = "det_1"
+    dither_sequence_random_offset: Literal[[True, False]] = True


Isn't this... "Literally" a boolean? ✔️

yes, but it's defined this way in the cerberus schema to get us a select dropdown in the UI... The effect in the library should be the same as if it was an Annotated[bool] right?

Fingel · 2025-10-02T16:36:58Z

src/aeonlib/ocs/lco/instruments.py

+class Lco0M4ScicamQhy600ConfigExtraParams(BaseModel):
+    model_config = ConfigDict(validate_assignment=True, extra='allow')
+    sub_expose: Literal[[False, True]] = False
+    sub_exposure_time: Optional[Annotated[float, Ge(15.0)]] = None


As per previous comment, this should be sub_exposure_time: Annotated[float, Ge(15.0)] | None = None

Fingel · 2025-10-02T16:37:48Z

README.md


 ```bash
-codegen/lco/generator.py instruments.json
+codegen/lco/generator.py {facility} instruments.json


Thank you this is a terrible omission

jnation3406 · 2025-10-02T18:22:22Z

Thanks Jon!

Crazy work on the jinja template there, that must have hurt your eyes.

While this does technically work, I was hoping for something more generic, as you alluded to in your commit message this only works for a subset of fields, and it gets more difficult for the others.

My view of this is that leaving extra_fields as a dict[Any, Any] is exactly how people are already using the API now. For example, if someone wants to use BLANCO, they have to somewhere (I don't even know where) look up the values for extra_fields and place them in their JSON request. The OCS API docs will not help them here. This is a fundamental downside to allowing your API to accept arbitrary data like that. So what Aeonlib is doing now isn't worse than the current API, it's on par with it.

So in my opinion, if we are going to try and work around arbitrary fields by parsing cerberus schemas, we should try to do it in the best possible way, otherwise what are we really gaining? A bunch of hardcoded tech debt for a small subset of fields.

I think we should do it the hard way.

I don't even know what the hard way is, or if it is really feasible. For instance, how would you put into the pydantic model that if they select the readout mode "fowler1", their exposure_time field has new limits of between 12 and 53 seconds? Those types of conditionals parsed out of the validation_schema seem very difficult to implement in any way in pydantic.

What we get from this simpler implementation is that any NEW fields imposed by an instrument_type at the configuration or instrument_config level (which happens a bunch for BLANCO) are represented here. This means the user of this library will get the typing help and checking on those new fields - they will see their default values in their json, and they will see their options and have them validated when making their request. If you are trying to make a TOM form based solely on the contents of AEONLib and not based on secret extra knowledge you have, then it will need these fields represented.

Fingel

This looks WAY better now, thanks!

Add in support for pulling out extra params fields defined in instrum…

d006edb

…ent_type validation_schema for OCS

jnation3406 requested a review from Fingel October 2, 2025 05:13

jchate6 added this to AEON Plus Oct 2, 2025

jchate6 moved this to Needs Review in AEON Plus Oct 2, 2025

Fingel requested changes Oct 2, 2025

View reviewed changes

Jon added 2 commits October 2, 2025 21:01

Moved validation_schema extra_params field parsing to the python side

0a998b5

Thanks ruff

dc15d3e

Fingel approved these changes Oct 2, 2025

View reviewed changes

Fix comment

49c94ff

Fingel merged commit 6eb80fe into main Oct 10, 2025
3 checks passed

github-project-automation bot moved this from Needs Review to Merged (to dev) in AEON Plus Oct 10, 2025

Add in support for pulling out extra params fields defined in instrum… #57

Add in support for pulling out extra params fields defined in instrum… #57

Uh oh!

Conversation

jnation3406 commented Oct 2, 2025

Uh oh!

Fingel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnation3406 commented Oct 2, 2025

Uh oh!

Fingel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants