[PNE-241] Data Manager support #943
Conversation
|
Note about how I PR, since I don't think I've done one for you before. |
src/citrine/_utils/functions.py
Outdated
| import warnings | ||
| warnings.filterwarnings('always', category=DeprecationWarning) |
There was a problem hiding this comment.
I think this is just here for testing, but just flagging to ensure we remove it before merge.
There was a problem hiding this comment.
Yep! Just testing. They weren't showing up in my notebook otherwise for some reason
| ) | ||
|
|
||
| def test_invalid_collection_construction(invalid_collection): | ||
| # assertion is within the construction of the invalid_collection |
There was a problem hiding this comment.
Fixtures should be used to construct data. If the construction is the test, it belongs in the test function, unless there's some very good reason.
There was a problem hiding this comment.
Got it. I'll fix this and my other occurrences of a similar pattern
| def __init__(self, project_id, session): | ||
| DatasetCollection.__init__(self, project_id=project_id, session=session) | ||
| def __init__(self, project_id, session, team_id): | ||
| DatasetCollection.__init__(self, team_id = team_id, project_id=project_id, session=session) |
There was a problem hiding this comment.
Hmmmmmmmm...not sure why this was never moved over to super. Mind making that change while you're here?
I appreciate it! The more feedback the better. For example, I didn't know that the standard is always |
| @@ -58,8 +58,6 @@ def __str__(self): | |||
| class ConditionTemplateCollection(AttributeTemplateCollection[ConditionTemplate]): | |||
| """A collection of condition templates.""" | |||
|
|
|||
| _path_template = 'projects/{project_id}/datasets/{dataset_id}/condition-templates' | |||
There was a problem hiding this comment.
👍 , go it, now it is a property on the DataConceptsCollection
src/citrine/resources/gemtables.py
Outdated
There was a problem hiding this comment.
@anoto-moniz here we will need to discuss how to pick the appropriate API endpoint for building the table depending on the desired execution scope (whole team or project), I've sent you a pm to start talking about it.
There was a problem hiding this comment.
It should be the same as the rest. Add support for a team_id parameter, and expose it on the TeamCollection, while deprecating the project_id parameter.
| dataset_uid=self.uid, | ||
| project_id=self.project_id | ||
| team_id=self.team_id |
There was a problem hiding this comment.
I see, even the team_id is not mandatory for the dataset's collection both the project and team initialize the collection with the corresponding team_id. This is ok.
| query = { | ||
| "criteria": [ | ||
| { | ||
| "datasets": str(self.dataset_id), |
| @@ -192,14 +193,44 @@ def __str__(self): | |||
| class FileCollection(Collection[FileLink]): | |||
| """Represents the collection of all file links associated with a dataset.""" | |||
|
|
|||
| _path_template = 'projects/{project_id}/datasets/{dataset_id}/files' | |||
| _path_template = 'teams/{team_id}/datasets/{dataset_id}/files' | |||
There was a problem hiding this comment.
Will the projects version of the endpoint be dead?
There was a problem hiding this comment.
We'll kill it after a while, but given that we are using the datasetId we can freely use the teams-based one for both project->dataset and team->dataset scenarios. It is going to return exactly the same data.
tests/resources/test_dataset.py
Outdated
There was a problem hiding this comment.
For each of these tests, we need to check that the projects version gives a DeprecatedWarning.
tests/resources/test_project.py
Outdated
There was a problem hiding this comment.
We should be checking that invoking the data related endpoints through ProjectCollection raises a deprecation warning.
anoto-moniz
changed the title
|
This work was already handled on #949 |
Citrine Python PR
Jira Ticket
PNE-241
Description
WIP draft for supporting data manager.
PR Type:
Adherence to team decisions