Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(workflows): Add support for simulation integration #1999

Merged
merged 35 commits into from
Dec 3, 2024
Merged

feat(workflows): Add support for simulation integration #1999

merged 35 commits into from
Dec 3, 2024

Conversation

lpereiracgn
Copy link
Contributor

@lpereiracgn lpereiracgn commented Oct 31, 2024 •
edited
Loading

Description

Please describe the change you have made.

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.
  • Changelog updated in CHANGELOG.md.
  • Version bumped. If triggering a new release is desired, bump the version number in _version.py and pyproject.toml per semantic versioning.

@lpereiracgn lpereiracgn marked this pull request as ready for review October 31, 2024 11:25
@lpereiracgn lpereiracgn requested review from a team as code owners October 31, 2024 11:25
doctrino
doctrino requested changes Nov 1, 2024
View reviewed changes
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.

Great to have simulation support in Workflows.

Does this not need the beta flag set in the API? Can you verify with the workflow team that this is not necessary?
image

In addition, lets avoid dict when we can. For the users of the SDK it is difficult to work with.

cognite/client/data_classes/workflows.py Outdated Show resolved Hide resolved
tests/tests_integration/test_api/test_data_workflows.py Outdated Show resolved Hide resolved
cognite/client/data_classes/workflows.py Outdated Show resolved Hide resolved
@lpereiracgn
Copy link
Contributor Author

Already confirmed the beta header is not necessary on the workflows' side. Also, the review comments you left were already addressed @doctrino

@codecov Codecov
Copy link

codecov bot commented Nov 29, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 96.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 90.49%. Comparing base (5d60284) to head (4ae69a2).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
cognite/client/data_classes/workflows.py 92.68% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1999      +/-   ##
==========================================
+ Coverage   90.48%   90.49%   +0.01%     
==========================================
  Files         139      141       +2     
  Lines       22418    22491      +73     
==========================================
+ Hits        20284    20354      +70     
- Misses       2134     2137       +3
Files with missing lines Coverage Δ
cognite/client/_version.py 100.00% <100.00%> (ø)
cognite/client/data_classes/__init__.py 100.00% <ø> (ø)
cognite/client/data_classes/simulators/__init__.py 100.00% <100.00%> (ø)
cognite/client/data_classes/simulators/runs.py 100.00% <100.00%> (ø)
cognite/client/data_classes/workflows.py 95.84% <92.68%> (-0.07%) ⬇️

... and 1 file with indirect coverage changes

doctrino
doctrino reviewed Dec 1, 2024
View reviewed changes
cognite/client/data_classes/simulators.py Outdated


@dataclass
class SimulationInputOverride(CogniteObject):
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the motivation to move this to a separate .py file?

Is it expected to be used with the simulator endpoints?

Copy link
Contributor

Choose a reason for hiding this comment

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

exactly. there will be many more simint classes added, @lpereiracgn is working on another PR for that

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to move this into /data_classes/simulators/<something>.py? See for example what we did for hosted_extractors/. The thinking is that with more resources we start to get overlapping names, so we want to separate them into "namespace" which in python would be different packages.

Copy link
Contributor

Choose a reason for hiding this comment

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

moved

cognite/client/data_classes/simulators.py Outdated Show resolved Hide resolved
cognite/client/data_classes/simulators.py Outdated Show resolved Hide resolved
cognite/client/data_classes/workflows.py Show resolved Hide resolved
cognite/client/data_classes/workflows.py Outdated Show resolved Hide resolved
doctrino
doctrino requested changes Dec 1, 2024
View reviewed changes
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.

It will be good to get this one in, but two things.

  1. Can you update the API docs to match what is actual there? This will cause a lot of confusion if it deviates.
  2. A few smaller things, see the comments.

doctrino
doctrino reviewed Dec 3, 2024
View reviewed changes
cognite/client/data_classes/simulators/runs.py
if TYPE_CHECKING:
from cognite.client import CogniteClient

_WARNING = FeaturePreviewWarning(api_maturity="General Availability", sdk_maturity="alpha", feature_name="Simulators")
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

@polomani polomani enabled auto-merge (squash) December 3, 2024 12:33
@polomani polomani added auto-update Will automatically keep PR up to date auto-merge and removed auto-merge labels Dec 3, 2024
@polomani polomani merged commit 126d0c9 into master Dec 3, 2024
16 checks passed
@polomani polomani deleted the POFSP-744-workflows branch December 3, 2024 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evertoncolling evertoncolling evertoncolling left review comments

@polomani polomani polomani left review comments

@doctrino doctrino doctrino approved these changes

Assignees
No one assigned
Labels
auto-update Will automatically keep PR up to date
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lpereiracgn @polomani @evertoncolling @doctrino