Feature/eng 5549 support ensemble runs via python sdk #143
Conversation
cowanwalls
requested review from
merschformann,
racquesta and
sebastian-quintero
There was a problem hiding this comment.
This is looking really good, congratulations! Aside from the specific comments, please add the new ensemble.py to the docs reference. For that there are two changes you need to make:
- In the
mkdocs.ymlfile in the root of the repo, add this reference in the appropriate location, which should be under- client.py: nextmv/reference/cloud/client.md, to keep the alphabetical order. You can simply copy that line and changeclientforensemble. - Because you are referencing a non-existent
ensemble.mdfile, you need to create it. Add it todocs/nextmv/reference/cloud. You can copy any of the existing files there, rename it, and modify the content to point to the correct reference::: nextmv.nextmv.cloud.ensemble.
Basically, whenever we add new files to the SDK, we need to add them to the technical reference of docs as well.
nextmv/nextmv/cloud/application.py
Outdated
| endpoint=f"{self.endpoint}/secrets/{secrets_collection_id}", | ||
| ) | ||
|
|
||
| def delete_ensemble_definition(self, ensemble_definition_id: str) -> None: |
There was a problem hiding this comment.
Nit: we try to organize the methods in this class alphabetically, so this one should be above delete_scenario_test.
nextmv/nextmv/cloud/application.py
Outdated
|
|
||
| def delete_ensemble_definition(self, ensemble_definition_id: str) -> None: | ||
| """ | ||
| Delete a secrets collection. |
There was a problem hiding this comment.
This is not accurate.
nextmv/nextmv/cloud/application.py
Outdated
| return False | ||
| raise e | ||
|
|
||
| def ensemble_definition(self, ensemble_definition_id: str) -> EnsembleDefinition: |
There was a problem hiding this comment.
Nit: same comment about ordering methods alphabetically inside the class.
nextmv/nextmv/cloud/application.py
Outdated
|
|
||
| return [Instance.from_dict(instance) for instance in response.json()] | ||
|
|
||
| def list_ensemble_definitions(self) -> list[EnsembleDefinition]: |
There was a problem hiding this comment.
Nit: same comment here about ordering alphabetically.
nextmv/nextmv/cloud/application.py
Outdated
| Returns | ||
| ------- | ||
| list['EnsembleDefinition'] |
There was a problem hiding this comment.
| list['EnsembleDefinition'] | |
| list[EnsembleDefinition] |
I think in this case backticks are not needed.
nextmv/nextmv/cloud/application.py
Outdated
| ---------- | ||
| id: str | ||
| ID of the ensemble defintion. | ||
| name: Optional[str] | ||
| Name of the ensemble definition. | ||
| description: Optional[str] | ||
| Description of the ensemble definition. | ||
| run_groups: Optional[list[RunGroup]] | ||
| Information to facilitate the execution of child runs. | ||
| rules: Optional[list[EvaluationRule]] | ||
| Information to facilitate the selection of | ||
| a result for the ensemble run from child runs. |
There was a problem hiding this comment.
Parameters should be in the same order they are expected in the function arguments.
nextmv/nextmv/cloud/application.py
Outdated
|
|
||
| response = self.client.request( | ||
| method="POST", | ||
| endpoint=f"{self.endpoint}/ensembles", |
There was a problem hiding this comment.
Nit: consider defining ensembles as an attribute of the class, similar to experiments_endpoint:
experiments_endpoint: str = "{base}/experiments"That way, you can use {self.ensembles_endpoint} as opposed to {self.endpoint}/ensembles here and in other places.
|
|
||
| return Instance.from_dict(response.json()) | ||
|
|
||
| def update_ensemble_definition( |
There was a problem hiding this comment.
Nit: same comment about alphabetical order.
| from .batch_experiment import ExperimentStatus as ExperimentStatus | ||
| from .client import Client as Client | ||
| from .client import get_size as get_size | ||
| from .ensemble import EnsembleDefinition as EnsembleDefinition |
There was a problem hiding this comment.
Strange, this should be ok.
|
@cowanwalls You should |
There was a problem hiding this comment.
My only leftover request is for this new file to be exposed as reference. Look at the cloud section in the mkdocs.yml index, you should see how to add the file as .md as well. This for docs to be up to date. Would really be awesome to have a dedicated page of a tutorial to handle ensemble definitions and runs in general.
cowanwalls
deleted the
feature/eng-5549-support-ensemble-runs-via-python-sdk
branch
2 participants
Description
Adds ensemble definition support.
Changes
nextmv/nextmv/cloud/ensemble.pynextmv/nextmv/cloud/__init__.pyToleranceTypein acceptance tests and Ensemble runs to a single type? Maybe via the addition of astatatics.pyfile innextmv/nextmv/cloud?nextmv/nextmv/cloud/application.pynew_versionnow defaults thenamefield to the value ofidifnameisNone(fixes unexpected 404)Testing
Manually tested these changes.