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

Catalog end-points #1460

Merged
merged 11 commits into from
Aug 21, 2024
Merged

Catalog end-points #1460

merged 11 commits into from
Aug 21, 2024

Conversation

Tansito
Copy link
Member

@Tansito Tansito commented Aug 16, 2024

Summary

This PR pretends to enable catalog end-points for our integration in IQP. For this integration we are creating two new end-points:

  • /api/v1/catalog: that returns all the public qiskit functions
  • /api/v1/catalog/:id: that returns a specific public qiskit functions

Details about the implementation

  • What is a "Public Qiskit Function"?

Basically a Public Qiskit Function it's a Qiskit Function that everyone should have access to view (but not run specifically), even not authenticated users.

For this we synced that all Qiskit Functions with the group "ibm-q/open/main" will be considered Public Qiskit Functions.

Due to this there some considerations to take into account:

  • These end-points are public, so no-authenticated users are able to call to them too.
  • There are 4 new fields:
    • serializer only available: it will say to the catalog if the user (if exists in the request) has access or not
    • type: it will identify the type of the function: Generic, Application or Circuit.
    • url, icon_ulr, documentation_url are basically fields that will contain URLs specific for the provider or the function
    • additional_info: contains a JSON structure with information for the catalog. Happy to discuss this approach, the decision behind it was that if the front-end requires a change we could easily change it without a new release or any change in the model.
  • We didn't introduce yet how to populate these fields. The idea is try to have for the next week the admin panel and populate the info there.

For the rest of the things there are no more news: this PR introduces a new view, serializers, tests and swagger documentation for the new end-points.

@Tansito Tansito requested review from psschwei, akihikokuroda and IceKhan13 and removed request for psschwei August 16, 2024 16:09
@psschwei
Copy link
Collaborator

These end-points are public, so no-authenticated users are able to call to them too.

Do we anticipate endusers calling these APIs, or are they solely for the catalog? If it's the latter, we should probably restrict their usage somehow (either authentication or network policy restrictions or ...)

@Tansito
Copy link
Member Author

Tansito commented Aug 16, 2024

Do we anticipate endusers calling these APIs, or are they solely for the catalog? If it's the latter, we should probably restrict their usage somehow (either authentication or network policy restrictions or ...)

Initially they are only designed for the catalog. There is a use case where a non-authenticated user in IQP could enter in the catalog page and it should be able to check the different public functions (in this case available is set to False automatically).

I don't see probably the authentication but network policy restriction can have a lot of sense to be honest. We could take a look later, agree.

Comment on lines -264 to -266

print("TEST: Program succeeds if all dependencies are allowlisted")

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you want to remove these? If so, that's ok -- I just added those so I could see which output went with which test (which ideally we'd have for all tests).

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have a strong opinion to be honest. In my case, I usually try to reduce the number of the outputs because I don't think they add value. I don't usually see for the test that passed if not for the test that it didn't and in that case pytest shows where is the error and its trace.

I just removed it because I thought it was a lost print between tests. If we think they add value happy to start adding them.

gateway/api/serializers.py Outdated Show resolved Hide resolved
gateway/api/serializers.py Outdated Show resolved Hide resolved
def test_catalog_retrieve_non_auth_user(self):
"""Tests catalog retrieve non-authenticated."""
url = reverse(
"v1:catalog-detail", args=["1a7947f9-6ae8-4e3d-ac1e-e7d608deec82"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure how the reverse works. Is this testing retrieve function?

Copy link
Member Author

@Tansito Tansito Aug 16, 2024

Choose a reason for hiding this comment

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

It's an utility from django, usually used in tests to not write every time the entire url:

https://docs.djangoproject.com/en/5.0/ref/urlresolvers/#reverse

In this case v1:catalog-detail points to retrieve but for example to access to the programs/get_by_title the reverse would be: v1:programs-get-by-title.

I usually get this info from this page:

Screenshot 2024-08-16 at 3 35 03 PM

I don't know, for me it's easier to manage it and we were using it before in other tests too: https://github.com/Qiskit/qiskit-serverless/blob/main/gateway/tests/api/test_v1_program.py#L22

Happy to hear feedback if you prefer not use it. Unify approaches for me it's better 👌

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know how

v1:catalog-detail points to retrieve

Doesn't v1:catalog-retrieve work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't v1:catalog-retrieve work?

Surprisingly not, I had the same doubt 😂

list, retrieve, update are built in methods created by DRF ViewSets. I suppose that something below attaches the retrieve method from DRF to the detail name in Django. From what I saw was the only one method that has a special name.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you tired just v1:catalog? I have no idea so why the detail work. Would you add comments on the Reverse with the catalog-detail?

Copy link
Member Author

Choose a reason for hiding this comment

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

Have you tired just v1:catalog?

Basically it follows the pattern: <namespace>:<name> where <name> is the name that django gives to the end-point (you can check an example in the image that I just shared a bit above).

Would you add comments on the Reverse with the catalog-detail?

Sure, I will 👍

@Tansito Tansito requested a review from psschwei August 16, 2024 19:40
Copy link
Collaborator

@psschwei psschwei left a comment

Choose a reason for hiding this comment

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

looks good from my side, will let Aki give final approval once his threads are resolved

Copy link
Collaborator

@akihikokuroda akihikokuroda left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@Tansito
Copy link
Member Author

Tansito commented Aug 19, 2024

Thanks @akihikokuroda , I'm just validating the format of the response before merge it and I'm almost have everything for the admin panel.

@Tansito
Copy link
Member Author

Tansito commented Aug 21, 2024

It seems everything looks so I'm going to merge to move forward with the admin-panel 👍

@Tansito Tansito merged commit 4b088ae into main Aug 21, 2024
10 checks passed
@Tansito Tansito deleted the iqp-catalog-access branch August 21, 2024 18:11
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.

3 participants