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

Second thoughts about list_services #521

Closed
msdemlei opened this issue Feb 8, 2024 · 8 comments
Closed

Second thoughts about list_services #521

msdemlei opened this issue Feb 8, 2024 · 8 comments
Labels
component: registry release blocker Issues that should block release: either critical bugs or required adjusments to new API

Comments

@msdemlei
Copy link
Contributor

msdemlei commented Feb 8, 2024

This isn't really a bug report or a wishlist item but rather a request to think about better ways. And it's still the aftermath of #505.

So, I think I don't like list_services() API, on grounds that it constructs potentially quite a few services, while it's quite clear that in the end likely only one of them will be used. Service construction, however, may be expensive, potentially retrieving capability documents or perhaps one day even VOSI tables.

Additionally, we may need to pass in sessions (allowing sessions in get_service will be part of the the upcoming global discovery PR), for instance for auth or timeouts. So, list_services would have to replicate that argument, too.

All that doesn't feel good. Hence, before everyone gets used to list_services, can we perhaps reconsider it? Perhaps we can discuss the various capabilities in .describe() and tell people to look there in tne lax=False error message of get_service?

@ManonMarchand, do you have any thoughts?

@ManonMarchand
Copy link
Member

I see the issue, but I don't like the idea of giving the information in describe(), that's a text and thus not machine readable.

What about returning the list of services as urls and descriptions without constructing the Service objetcs? I'd find that useful, and we avoid the costly service creation.

@bsipocz bsipocz added component: registry Affects-dev To be used for follow-up PRs for new engancements or bugs only present on dev labels Feb 8, 2024
@bsipocz bsipocz added this to the v1.6 milestone Feb 8, 2024
@bsipocz
Copy link
Member

bsipocz commented Feb 8, 2024

I milestone this as 1.6 as this should ideally be resolved before #505 ends up in a release

edit: in fact I think it's useful to add a new label category for release blockers: either critical bugs or required changes/prefered clarifications for new API where requiring a resolution before release would have the potential of saving the trouble of deprecation and changing API after release.

@bsipocz bsipocz added release blocker Issues that should block release: either critical bugs or required adjusments to new API and removed Affects-dev To be used for follow-up PRs for new engancements or bugs only present on dev labels Feb 8, 2024
@msdemlei
Copy link
Contributor Author

msdemlei commented Feb 9, 2024 via email

@ManonMarchand
Copy link
Member

ManonMarchand commented Feb 9, 2024

Our use case (not in production yet) was to get all services of a given type for a resource automatically, so not really for humans even if they might want to peek at the result of the method. I can still do that by calling to_service on a pretty list, but not by parsing the output of describe()

Why would we want to look at Interface? I think I missed something here. My idea was that the returned list should not contain Interface instances but a small inner dataclass that could be fed to to_service. Something along:

from dataclasses import dataclass

@dataclass 
class ListServiceItem:
    url: str
    description: str
    servicetype: str

And then either define a to_service for this dataclass or edit the existing to_service to accept a ListServiceItem too, depending on what it less ugly when writing it.

@msdemlei
Copy link
Contributor Author

msdemlei commented Feb 9, 2024 via email

@ManonMarchand
Copy link
Member

I see. I can explore and see what works, but not before next wednesday. Would that work for you?

@msdemlei
Copy link
Contributor Author

msdemlei commented Feb 9, 2024 via email

@ManonMarchand
Copy link
Member

I think this one can be closed.

@msdemlei msdemlei closed this as completed Mar 4, 2024
@bsipocz bsipocz removed this from the v1.6 milestone Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: registry release blocker Issues that should block release: either critical bugs or required adjusments to new API
Projects
None yet
Development

No branches or pull requests

3 participants