-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: JIRA-1710 (minor) completing registry api L2 and L3 interfaces with support for admin, general and other endpoints. #18
Conversation
…ng integration tests, so I am pivoting to complete that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good.
I have put approve but I think there's some minor improvements you could make.
One that I didn't put in comment is that the Admin functionality of the registry should really be
- general stuff e.g. import/export
- subtype specific e.g. delete
You shouldn't have to specify the subtype in the client interface, it should follow pattern client.model.admin.delete(id="1234")
rather than client.admin.delete(id="1234", subtype=Model)
If you have an idea of a way to do this without too much extra work maybe worth doing to keep the interface consistent, but otherwise not a huge deal.
I think Version endpoint can be moved to the base class which will reduce some duplication.
Otherwise minor changes then all good. Great work!
@@ -11,16 +11,19 @@ | |||
Date By Comments | |||
---------- --- --------------------------------------------------------- | |||
|
|||
28-06-2024 | Parth Kulkarni | Completion of L2 Interface of Registry with General, Admin and Other endpoints. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice :)
RegistryImportResponse | ||
The response containing the result of the restore operation. | ||
""" | ||
endpoint = self._build_endpoint(RegistryAdminEndpoints.POST_ADMIN_IMPORT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this endpoint right? same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep your right, that's fixed now.
@@ -145,3 +401,484 @@ async def update_item(self, id: str, reason: Optional[str], item_subtype: ItemSu | |||
model=update_response_model, | |||
url=endpoint, | |||
) | |||
|
|||
async def list_items(self, list_items_payload: GeneralListRequest, item_subtype: ItemSubType, update_model_response: Type[BaseModelType]) -> BaseModelType: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something funny going on here - update_model_response? no update going on here. And is the subtype list request the same as general list request?
url=endpoint | ||
) | ||
|
||
async def seed_item(self, item_subtype: ItemSubType, update_model_response: Type[BaseModelType]) -> BaseModelType: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here with update_model_response - the name is supposed to reflect the type e.g. in this case it would be seed_model_response
url=endpoint | ||
) | ||
|
||
async def create_item(self, create_item_request: DomainInfoBase, item_subtype: ItemSubType, update_model_response: Type[BaseModelType]) -> BaseModelType: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just need to be careful about typing the create item as domain info base - it might be okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again with update_model_response
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Domain info base should be fine, since in L3 each method has their own unique domain base e.g study, model run, and they all inherit from DomainInfoBase.
Helps to keep this method as "broad" as possible, and plus i tested creating a model and it seemed fine,
|
||
DEFAULT_CONFIG_FILE_NAME = "registry-api.env" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you make jira ticket to add config file endpoint to the other APIs - they all have it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
restore_request=restore_request | ||
) | ||
|
||
async def generate_config_file(self, required_only: bool = True, file_path: Optional[str] = None, write_to_file: bool = False) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
registry_client : RegistryClient | ||
The registry client to use for registry interactions. | ||
""" | ||
super().__init__(auth=auth, config=config, registry_client=registry_client, item_subtype=ItemSubType.ORGANISATION) | ||
|
||
async def fetch(self, id: str, seed_allowed: Optional[bool] = None) -> OrganisationFetchResponse: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be nice if python had generic typed classes !
|
||
def __init__(self, auth: AuthManager, config: Config, registry_client: RegistryClient) -> None: | ||
return await self._registry_client.seed_item( | ||
item_subtype=ItemSubType.PERSON, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
were you planning to use self.item_subtype consistently here to make the copy a bit easier to copy/paste if needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw you had previously
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, better to use the self variable, easier to change in the future as well.
async def version_item(self, version_request: VersionRequest) -> VersionResponse: | ||
""" | ||
Versions a model in the registry | ||
|
||
Parameters | ||
---------- | ||
version_request : VersionRequest | ||
The version request | ||
|
||
Returns | ||
------- | ||
VersionResponse: The version response | ||
""" | ||
|
||
return await self._registry_client.version( | ||
version_request=version_request, | ||
item_subtype=ItemSubType.MODEL | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought Version could be part of the base calss?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certain subtypes don't use version, hence why it's not in base class.
I have done this, by internally having a general fetch in the delete method. |
Jira-1710 (Major/Minor/Bug Fix): Minor
JIRA Ticket 1710
Checklist
Description
Completion of Registry API with L2 and L3 interfaces support for admin, general and other endpoints.
I have also done some ad-hoc testing successfully.
Notes for reviewer
... (Optional) Notes here...