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.

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

Simulator resources creation #2041

Draft
wants to merge 33 commits into
base: simulator-resources
Choose a base branch
from

Conversation

abdullah-cognite
Copy link

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.

@abdullah-cognite abdullah-cognite changed the base branch from master to simulator-resources November 21, 2024 13:58
Comment on lines 223 to 229
return self._create_multiple(
list_cls=SimulatorModelList,
resource_cls=SimulatorModel,
items=models,
input_resource_cls=SimulatorModelWrite,
resource_path="/simulators/models",
)
Copy link
Contributor

Choose a reason for hiding this comment

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

for this operation to work you have to set limit=1 otherwise this request will fail when multiple models are passed in.

Copy link
Contributor

Choose a reason for hiding this comment

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

Set self._CREATE_LIMIT = 1 in __init__ and this will be taken care of automatically.

Copy link
Contributor

Choose a reason for hiding this comment

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

The base resource_path here should be set as a class variable, I believe _RESOURCE_PATH. Then you can use:

resource_path=self._RESOURCE_PATH,
# ...and similarity for other endpoints, e.g.:
resource_path=self._RESOURCE_PATH + "/revisions",

Comment on lines 293 to 300
return self._create_multiple(
list_cls=SimulatorModelList,
resource_cls=SimulatorModel,
items=revisions,
input_resource_cls=SimulatorModelRevisionWrite,
resource_path="/simulators/models/revisions",
)

Copy link
Contributor

Choose a reason for hiding this comment

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

for this operation to work you have to set limit=1 otherwise this request will fail when multiple models are passed in.

Comment on lines 320 to 322
return self._update_multiple(
list_cls=SimulatorModelList, resource_cls=SimulatorModel, update_cls=SimulatorModelUpdate, items=item
)
Copy link
Contributor

Choose a reason for hiding this comment

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

_update_multiple doesn't accept limit param (basically the chunk size).
_UPDATE_LIMIT is set to 1000 but our API only supports one at a time.

However, if we can't customize _UPDATE_LIMIT like the create_multiple, the request will fail, but that should not be a big deal. We can keep this as a it is

@lpereiracgn lpereiracgn mentioned this pull request Dec 10, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants