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

[IO-1279] Functions to move items around stages #640

Merged
merged 14 commits into from
Jul 25, 2023
Merged

[IO-1279] Functions to move items around stages #640

merged 14 commits into from
Jul 25, 2023

Conversation

Nathanjp91
Copy link
Contributor

Problem

Don't have any way to move items around workflows in darwin-py at the moment

Solution

Add in functions to stage items as well as backend core functions to move items

Changelog

Can move items in workflows, see item's attached to a stage and manage workflows better

@linear
Copy link

linear bot commented Jul 21, 2023

IO-1279 Core Backend: Push Item to Stage

Need to scope out the API required

@@ -155,7 +155,7 @@ def _setup_session(self, retries: Retry) -> None:

@property
def headers(self) -> Dict[str, str]:
http_headers: Dict[str, str] = {"Content-Type": "application/json"}
http_headers: Dict[str, str] = {"Content-Type": "application/json", "Accept": "application/json"}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was required for the stage api, but if we need to set this for other api's we'll need to think about a syntax for setting headers of the client, either passing headers as an optional parameter to the client.get(...)/REST functions or changing headers before certain API calls

@@ -172,7 +172,7 @@ def _generic_call(self, method: Callable, endpoint: str, payload: Optional[dict]
endpoint = self._sanitize_endpoint(endpoint)
url = self.config.api_endpoint + endpoint
if payload is not None:
response = method(url, payload)
response = method(url, json=payload)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

data didn't work here for some reason

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it only works if the data type isn't "app/json" or on certain methods.

@@ -193,5 +193,21 @@ def where(self, *args: object, **kwargs: str) -> Query[T]:
def collect(self) -> List[T]:
raise NotImplementedError("Not implemented")

def collect_one(self) -> T:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both here added as dogfooding session with Simon

@@ -173,7 +179,7 @@ class Workflow(DefaultDarwin):
inserted_at: datetime
updated_at: datetime

dataset: WFDataset
dataset: Optional[WFDataset]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some workflows on platform don't have a dataset attached, prevents collecting these items if it's required field. Although intuitively you would think it should be

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, intuitively I did think so! Good catch



class MetaBase(Generic[R]):
_item: Optional[R]
client: Client

def __init__(self, client: Client, item: Optional[R] = None) -> None:
def __init__(self, client: Client, item: Optional[R] = None, meta_params: Optional[Param] = None) -> None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

meta_params have to be added to the MetaBase here so that chaining can happen properly with backend API calls.
client.team.workflows.first().stages.first().push_items_to_next_stage()

pushing items to stage requires other parameters like workflow_id, dataset_id, team_slug. This need to be passed from Meta -> Query -> Meta as it goes down the chain

@Nathanjp91 Nathanjp91 marked this pull request as ready for review July 25, 2023 09:23
Copy link
Contributor

@owencjones owencjones left a comment

Choose a reason for hiding this comment

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

Yeah, looks good - I've commented a lot, but there's nothing that needs changing.

@@ -172,7 +172,7 @@ def _generic_call(self, method: Callable, endpoint: str, payload: Optional[dict]
endpoint = self._sanitize_endpoint(endpoint)
url = self.config.api_endpoint + endpoint
if payload is not None:
response = method(url, payload)
response = method(url, json=payload)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it only works if the data type isn't "app/json" or on certain methods.

f"/v2/teams/{team_slug}/items/ids",
QueryString({"workflow_stage_ids": str(stage_id), "dataset_ids": str(dataset_id)}),
)
assert type(response) == dict
Copy link
Contributor

Choose a reason for hiding this comment

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

Could consider using the other function we have for this which is impervious to __DEBUG__ flag, but not the most important now - esp if this is just for making mypy happy!

f"/v2/teams/{team_slug}/items/ids",
QueryString({"not_statuses": "archived,error", "sort[id]": "desc", "dataset_ids": str(dataset_id)}),
)
assert type(response) == dict
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as my other (confusingly later) comment - could be assertis or just an if, but dw if this is just mypy cheering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's mostly for mypy type checking, but we can use the assert_is in these cases too.


Returns
-------
None
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't return None

@@ -173,7 +179,7 @@ class Workflow(DefaultDarwin):
inserted_at: datetime
updated_at: datetime

dataset: WFDataset
dataset: Optional[WFDataset]
Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, intuitively I did think so! Good catch

self.client = client
self._item = item
self.meta_params = meta_params or {}
Copy link
Contributor

Choose a reason for hiding this comment

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

There's some argument for this being or dict() just because {} is also the syntax for set delimitation, but now I'm really scraping the barrel I guess.

@@ -1,10 +1,17 @@
from typing import List, Optional, Tuple, Union
from __future__ import annotations
Copy link
Contributor

Choose a reason for hiding this comment

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

With this, I tend to just quote the returnable I need it for, but this is valid too, especially with our current support matrix.

@@ -155,3 +178,20 @@ def _validate_slug(slug: str) -> None:

VALID_SLUG_CHARS = "abcdefghijklmnopqrstuvwxyz0123456789-_"
assert_is(all(c in VALID_SLUG_CHARS for c in slug_copy), "slug must only contain valid characters")

def upload_files(
Copy link
Contributor

Choose a reason for hiding this comment

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

Good move using the CLI function - I was looking at which to do when I went sick last week.


def push_from_dataset_stage(self) -> WorkflowMeta:
assert self._item is not None
Copy link
Contributor

Choose a reason for hiding this comment

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

Are all of these to satiate mypy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pretty much

@@ -14,9 +14,10 @@ def collect(self) -> List[StageMeta]:
if not self.meta_params:
raise ValueError("Must specify workflow_id to query stages")
workflow_id: UUID = self.meta_params["workflow_id"]
meta_params = self.meta_params.copy()
Copy link
Contributor

Choose a reason for hiding this comment

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

Pass-by-ref was mutilating params as they were passed around?

@Nathanjp91 Nathanjp91 merged commit fae6e41 into master Jul 25, 2023
9 checks passed
@Nathanjp91 Nathanjp91 deleted the IO-1279 branch November 8, 2023 10:42
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.

None yet

2 participants