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

feat: JIRA-1710 (minor) completing registry api L2 and L3 interfaces with support for admin, general and other endpoints. #18

Merged
merged 7 commits into from
Jun 28, 2024

Conversation

parth-kulkarni1
Copy link
Collaborator

Jira-1710 (Major/Minor/Bug Fix): Minor

JIRA Ticket 1710

Checklist

  • If tests are required for this change, are they implemented?
  • Are user documentation changes required, if so, is there a task to track it and/or is it completed?
  • If developer/system documentation updates are required, is there a task to track it and/or is it completed?
  • At least one developer has reviewed this change (unless PR is being used to mark a commit point without need for review)?

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...

@parth-kulkarni1 parth-kulkarni1 changed the title feat: JIRA 1710 completing registry api L2 and L3 interfaces with support for admin, general and other endpoints. feat: JIRA-1710 (minor) completing registry api L2 and L3 interfaces with support for admin, general and other endpoints. Jun 28, 2024
Copy link
Contributor

@PeterBaker0 PeterBaker0 left a 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.
Copy link
Contributor

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)
Copy link
Contributor

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

Copy link
Collaborator Author

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:
Copy link
Contributor

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:
Copy link
Contributor

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:
Copy link
Contributor

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?

Copy link
Contributor

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

Copy link
Collaborator Author

@parth-kulkarni1 parth-kulkarni1 Jun 28, 2024

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"
Copy link
Contributor

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

Copy link
Collaborator Author

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:
Copy link
Contributor

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:
Copy link
Contributor

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,
Copy link
Contributor

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?

Copy link
Contributor

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

Copy link
Collaborator Author

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.

Comment on lines 972 to 989
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
)
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

@parth-kulkarni1
Copy link
Collaborator Author

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!

I have done this, by internally having a general fetch in the delete method.

@parth-kulkarni1 parth-kulkarni1 merged commit cd2b4e9 into main Jun 28, 2024
3 checks passed
@parth-kulkarni1 parth-kulkarni1 deleted the feat-1710-completing-registry-api branch August 22, 2024 00:53
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.

2 participants