-
Notifications
You must be signed in to change notification settings - Fork 36
feat(simint): make simulator_integration_external_id optional on runs, routines and routine revisions
#2420
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2420 +/- ##
==========================================
- Coverage 91.28% 91.27% -0.01%
==========================================
Files 192 192
Lines 26184 26184
==========================================
- Hits 23901 23900 -1
- Misses 2283 2284 +1
🚀 New features to boost your workflow:
|
|
you need to put a bit more explanation into the description, as this will be reviewed by pysdk maintainers. |
|
/gemini review |
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.
Code Review
This pull request modifies the simulator_integration_external_id field across several simulator-related data classes, including SimulatorRoutineRevision, SimulatorRoutine, and SimulationRun. The changes involve updating the type hints for this field from str to str | None in class definitions and __init__ methods, and modifying the _load methods to use resource.get("simulatorIntegrationExternalId") instead of direct dictionary access to gracefully handle cases where the field might be missing. Corresponding unit tests have been added to verify that these classes correctly load instances both with and without the simulator_integration_external_id.
polomani
left a comment
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.
The field remains required for write operation (SimulatorRoutineWrite) because the backend logic still requires it. Once the backend supports optional simulatorIntegrationExternalId in write operations, we can update the write classes accordingly.
this sentence is not true, it's stil required on the API but is optional in SDK now
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.
LGTM, just one comment in need of addressing
simulator_integration_external_id optional on runs, routines and routine revisions
haakonvt
left a comment
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.
🦄
…es, revisions and runs (#1346) `simulatorIntegrationExternalId` is a required field at the moment, which means that it has to be associated with a connector since the creation of the resource, to make it flexible to decide after creation which connector can pick it up, we're making that field optional in this PR. Ref: cognitedata/cognite-sdk-python#2420
Description
Currently, the
simulatorIntegrationExternalIdfield is required in the response objects for routine, routine revision, and simulation run endpoints. This forces users to bind a routine to a specific connector at creation time.We want to make
simulatorIntegrationExternalIdoptional in routine/routine revision creation, so users can create routines not tied to a connector. The binding will happen at runtime (simulation run will get the connector assigned at some point).This PR only makes the field optional in response objects of
SimulationRun,SimulatorRoutine,SimulatorRoutineRevision.Since
SimulatorRoutineWriteextendsSimulatorRoutineCore(which now acceptsstr | None), the write class also technically accepts None values. However, the backend API currently still requires this field for write operations, so users should continue to provide it when creating routines. Once the backend supports optionalsimulatorIntegrationExternalIdin writes, we can update the documentation accordingly.Note: Since simint pysdk is under alpha, we are doing this breaking change.
Changes
simulator_integration_external_id: strtosimulator_integration_external_id: str | NoneinSimulationRun,SimulatorRoutineCore,SimulatorRoutineRevision.resource["simulatorIntegrationExternalId"]toresource.get("simulatorIntegrationExternalId")to safely handle missing or None values.Checklist:
If a new method has been added it should be referenced in cognite.rst in order to generate docs based on its docstring.