Skip to content

Conversation

lpereiracgn
Copy link
Contributor

@lpereiracgn lpereiracgn commented Sep 19, 2025

Description

This PR extends the Simulator Runs API to support two modes for running simulations:

  1. Routine-based mode (existing): Run by specifying only routine_external_id
  2. Revision-based mode (new): Run by specifying both routine_revision_external_id and model_revision_external_id

Co-Authored: @evertoncolling . Ref.

Checklist:

  • Tests added/updated.
  • Documentation updated. Documentation is generated from docstrings - these must be updated according to your change.
    If a new method has been added it should be referenced in cognite.rst in order to generate docs based on its docstring.

Copy link

codecov bot commented Sep 19, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.80%. Comparing base (dcbc82e) to head (8a205ba).
⚠️ Report is 15 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2339      +/-   ##
==========================================
+ Coverage   90.68%   90.80%   +0.12%     
==========================================
  Files         169      170       +1     
  Lines       25186    25685     +499     
==========================================
+ Hits        22839    23323     +484     
- Misses       2347     2362      +15     
Files with missing lines Coverage Δ
cognite/client/_api/simulators/routines.py 93.10% <100.00%> (+0.51%) ⬆️
cognite/client/data_classes/simulators/runs.py 99.02% <100.00%> (+0.07%) ⬆️

... and 15 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@polomani polomani left a comment

Choose a reason for hiding this comment

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

mostly nitpicks, should be easy to fix

polomani
polomani previously approved these changes Sep 24, 2025
self.model_revision_external_id = model_revision_external_id


class SimulationRunWrite(SimulationRunCore):
Copy link
Contributor

Choose a reason for hiding this comment

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

What about making two subclasses here? Something like SimulationRunByRoutineWrite and SimulationRunByRevisionWrite. I think that will give a better user experience and you do not have to have a bunch of conditionals. It is also more aligned with the API that has two different objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried this approach and IMO the UX doesn't look much cleaner than it is at the moment.

For example: Some time ago we had to do additional imports for filtering https://github.com/cognitedata/cognite-sdk-python/pull/2148/files and we removed them.

In this case, if the user reaches one of the errors based on the conditionals, fixing it would be much faster than looking for the right class to execute a simulation. It was getting confusing just writing the docstrings for this.

Copy link
Contributor

@doctrino doctrino left a comment

Choose a reason for hiding this comment

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

See suggestions, will review the rest when those are addressed.

doctrino
doctrino previously approved these changes Oct 2, 2025
Copy link
Contributor

@doctrino doctrino left a comment

Choose a reason for hiding this comment

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

Just one nitpick

@lpereiracgn lpereiracgn dismissed stale reviews from doctrino and polomani via 8a205ba October 3, 2025 10:53
@polomani polomani added the waiting-for-risk-review Waiting for a member of the risk review team to take an action label Oct 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-for-risk-review Waiting for a member of the risk review team to take an action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants