Simulators API #2006
Simulators API #2006
Conversation
| from cognite.client.data_classes._base import CogniteFilter | ||
|
|
||
|
|
||
| class SimulatorIntegrationFilter(CogniteFilter): |
There was a problem hiding this comment.
Should these filters have some docstrings? @haakonvt
There was a problem hiding this comment.
Ideally yes, but I wouldn't bother. Instead, add a usage example to the docstrings where applicable so that people see how to use them 🚀
|
I see we are missing Simulation Data and Simulator Logs. We can take it in a follow up PR, but they could fell into the umbrella of read endpoints. |
Co-authored-by: Everton Colling <[email protected]>
Co-authored-by: Everton Colling <[email protected]>
Co-authored-by: Everton Colling <[email protected]>
Co-authored-by: Everton Colling <[email protected]>
Co-authored-by: Everton Colling <[email protected]>
Co-authored-by: Everton Colling <[email protected]>
Co-authored-by: Everton Colling <[email protected]>
Co-authored-by: Everton Colling <[email protected]>
Co-authored-by: Everton Colling <[email protected]>
Co-authored-by: Everton Colling <[email protected]>
Co-authored-by: Everton Colling <[email protected]>
Co-authored-by: Everton Colling <[email protected]>
Co-authored-by: Everton Colling <[email protected]>
Co-authored-by: Everton Colling <[email protected]>
I agree, this can go as a follow-up after we merge this one |
Co-authored-by: Everton Colling <[email protected]>
Co-authored-by: Everton Colling <[email protected]>
Co-authored-by: Everton Colling <[email protected]>
Co-authored-by: Everton Colling <[email protected]>
Co-authored-by: Everton Colling <[email protected]>
There was a problem hiding this comment.
Initial round of review (I haven't looked at most of data_classes/simulators...).
First and foremost I'd like to say that there is some solid work here!! Really looking forward to when this is released 💯
Some general notes:
- This PR is just way too large for the review process to be pleasant for any of us. I would greatly encourage you to split this into several pieces, then we can make one release at the end.
- Please hold off with adding tests until you have all the endpoints you need
- We typically add
__call__andlistat the same time then showcase the iterative approach in the list-docstring.
| Retrieves a list of simulation runs that match the given criteria | ||
|
|
||
| Args: | ||
| limit (int): The maximum number of simulation runs to return. |
There was a problem hiding this comment.
I almost agree with @evertoncolling comment above 😄
| limit (int): The maximum number of simulation runs to return. | |
| limit (int | None): Maximum number of assets to return. Defaults to 25. Set to -1, float("inf") or None to return all items. |
| List simulation runs: | ||
|
|
||
| >>> from cognite.client import CogniteClient | ||
| >>> client = CogniteClient() | ||
| >>> res = client.simulators.runs.list() |
There was a problem hiding this comment.
Need one example showing where to import SimulationRunsFilter and how to use it (can include the sort in the same example)
| if isinstance(sort, CreatedTimeSort): | ||
| sort_by = [CreatedTimeSort.load(sort).dump()] | ||
| elif isinstance(sort, SimulationTimeSort): | ||
| sort_by = [SimulationTimeSort.load(sort).dump()] |
There was a problem hiding this comment.
This should be enough? Also see my other comment regarding these sort-classes.
| if isinstance(sort, CreatedTimeSort): | |
| sort_by = [CreatedTimeSort.load(sort).dump()] | |
| elif isinstance(sort, SimulationTimeSort): | |
| sort_by = [SimulationTimeSort.load(sort).dump()] | |
| if isinstance(sort, CreatedTimeSort | SimulationTimeSort): | |
| sort_by = [sort.dump(camel_case=True)] |
| class PropertySort(CogniteSort): | ||
| def dump(self, camel_case: bool = True) -> dict[str, Any]: | ||
| dumped = super().dump(camel_case=camel_case) | ||
| dumped["property"] = self.property | ||
| return dumped | ||
|
|
||
|
|
||
| class CreatedTimeSort(PropertySort): | ||
| def __init__( | ||
| self, | ||
| property: Literal["createdTime"] = "createdTime", | ||
| order: Literal["asc", "desc"] = "asc", | ||
| ): | ||
| super().__init__(property, order) | ||
|
|
||
|
|
||
| class SimulationTimeSort(PropertySort): | ||
| def __init__( | ||
| self, | ||
| property: Literal["simulationTime"] = "simulationTime", | ||
| order: Literal["asc", "desc"] = "asc", | ||
| ): | ||
| super().__init__(property, order) |
There was a problem hiding this comment.
Let's merge these three classes into one. May I suggest we do sort similar to existing classes? E.g. check out SortableAssetProperty and AssetSort
| return self._list( | ||
| method="POST", | ||
| limit=limit, | ||
| url_path="/simulators/runs/list", |
There was a problem hiding this comment.
Not needed as this follows the API design guidelines
| url_path="/simulators/runs/list", |
| def truncated_uuid4(): | ||
| return str(uuid.uuid4())[:8] | ||
|
|
||
|
|
||
| model_unique_external_id = f"{resource_names['simulator_model_external_id']}_{truncated_uuid4()}" | ||
| model_revision_unique_external_id = f"{resource_names['simulator_model_revision_external_id']}_{truncated_uuid4()}" | ||
| simulator_routine_unique_external_id = f"{resource_names['simulator_routine_external_id']}_{truncated_uuid4()}" |
There was a problem hiding this comment.
This won't work with how we run tests (xdist package). These need to be put behind a fixture so that dependent tests agree on what the random variable is.
Also, you can just import random_string from utils?
| # check if file already exists | ||
| file = cognite_client.files.retrieve(external_id=seed_resource_names["simulator_model_file_external_id"]) | ||
| if (file is None) or (file is False): |
There was a problem hiding this comment.
| # check if file already exists | |
| file = cognite_client.files.retrieve(external_id=seed_resource_names["simulator_model_file_external_id"]) | |
| if (file is None) or (file is False): | |
| file = cognite_client.files.retrieve(external_id=seed_resource_names["simulator_model_file_external_id"]) | |
| if not file: |
There was a problem hiding this comment.
You should add a comment about what seed is (people like me would interpret it as something to make RNGs deterministic)
| cognite_client.post( | ||
| f"/api/v1/projects/{cognite_client.config.project}/simulators", | ||
| json={"items": [simulator]}, | ||
| ) |
There was a problem hiding this comment.
Don't add the test before you've added the create endpoint to save yourself some refactoring
| simulators = cognite_client.simulators.list() | ||
| simulator_exists = len(list(filter(lambda x: x.external_id == simulator_external_id, simulators))) > 0 |
There was a problem hiding this comment.
Is there no retrieve you can use? If you only have list you can do:
simulator_exists = simulators.get(external_id=simulator_external_id)
which is a O(1) lookup instead of O(N)
|
One more note, please don't push each commit one by one; we have quite an expensive test suite and we shouldn't run it willy-nilly 😄 |
Description
Support for Simulators API - Read only.
Supported Resources:
PR for write support #2041