Use ontoportal-client classes in BioPortal and AgroPortal implementations by pkalita-lbl · Pull Request #204 · INCATools/ontology-access-kit

pkalita-lbl · 2022-07-21T21:50:44Z

These changes introduce an OntoPortalInterface base class. It expects subclasses to populate the OntoPortalClientClass and api_key_name class variables. OntoPortalInterface then handles all of the implementation details of constructing the OntoPortalClient instance and using it to make requests.

Fixes #168

…ions These changes introduce an `OntoPortalInterface` base class. It expects subclasses to populate its `OntoPortalClientClass` and `api_key_name` class variables. `OntoPortalInterface` then handles all of the implementation details of constructing the `OntoPortalClient` instance and using it to make requests.

cmungall

I wasn't thinking of OntoPortal as an interface, but rather as a parent class for some implementations.

A few other things to bear in mind:

ontoportal-client is still young, I figure the methods it exposes may vary over time, which will mean a bit more coordinating and perhaps some version pinning
we don't have a great testing strategy for bioportal and friends yet. I run the tests locally using my API key, but gh actions will skip them (I think). We need to set up Mock unit tests, and have separate integration tests. This will all come in time but just be aware that there is more potential to temporarily break things here!

pkalita-lbl · 2022-07-22T00:05:49Z

I agree there is some potential for confusion here about OntoPortalInterface; it's not exactly an interface in the same way the other classes in the interfaces package are. I considered putting it in the implementations package, but it's also not quite an implementation that can can be used on its own. It's a bit neither-here-nor-there, but thinking about it now it probably belongs more in implementations. I'll make that change before merging.

Definitely on the same page as you about testing. I also run them locally using my API key, and you're right that they are skipped when run in the GitHub workflow. Mocking can be a headache but it's better than nothing. I can open an issue about that.

cthoyt · 2022-07-22T00:18:10Z

The ontoportal client has some pretty slick testing set up, you can see how the combination of api keys set as GitHub actions secrets and the unified lookup with pystow simplifies it pretty nicely

pkalita-lbl · 2022-07-22T00:28:55Z

That's also a good idea @cthoyt. I'll take a closer look at that when I work on #205.

I've updated this branch to replace oaklib.interfaces.ontoportal_interface.OntoPortalInterface with oaklib.implementations.ontoportal.ontoportal_implementation_base.OntoPortalImplementationBase. I think that makes it more clear what the purpose of that class is.

Maybe long term we want to put the BioPortal, AgroPortal, etc implementations into the oaklib.implementations.ontoportal package as well?

cthoyt · 2022-07-22T00:36:01Z

@pkalita-lbl just pushed a new ontoportal-client release (https://github.com/cthoyt/ontoportal-client/releases/tag/v0.0.3), that also does more of the things that you have in your get_response and get_json wrappers to reduce more code

cthoyt · 2022-07-22T00:38:26Z

+        response = self._get_response(*args, **kwargs)
+        return response.json()
+
+    def ontologies(self) -> Iterable[CURIE]:


the type annotation here for the result is misleading. the acronyms are prefixes, not CURIEs

good point, I think this is something that may need a deeper fix as part of a separate PR through

I think I'm with Chris on this one. It's not good, but it's also not an isolated thing and would probably be handled better in a separate PR.

cthoyt · 2022-07-22T16:50:24Z

-        self.client = self.OntoPortalClientClass(api_key=api_key)
+        if not self.ontoportal_client_class:
+            raise NotImplementedError("ontoportal_client_class not specified")
+        api_key = get_apikey_value(self.ontoportal_client_class.name)


the API key getting is handled by the class itself, so you can skip this completely from the OAK side

Yeah I saw that ontoportal_client manages API keys as well but as I understand it that would require the user to create either an environment variable or a file under ~/.config. The OAK command line interface has a set-apikey subcommand that I think we need to continue to support. So I thought it was better to continue to let OAK manage its own key storage.

In #142 I made it much more integrated with PyStow, since it's a pain to have to use several competing systems when there's already something ready to go out of the box. If someone wants to put effort into unmucking the testing (#173), then it will be possible to finish that PR, since right now the logging is too much of a mess to even figure it out. If I had to guess though, it might be because PRs from forks don't get access to the secrets in the GHA workflows

I can talk to Sujay about #173 since he currently is assigned and see if has any progress on it. If not maybe I can take it over from him. Also after I commented on #142 Chris gave me admin rights on this repo, so I'd be happy to work with you on that one. But as far as these changes go are you okay with decoupling them from the PyStow work? I'm not sure how much effort that will be, but does it make sense to just get this in now?

All of the code is simple, the only real effort is coordination :p

I can clean this up later, it’s not a blocker for your PR

Definitely appreciate all your feedback. I'm going to merge this now, but please get in touch with me next week (obo-community slack maybe?) and we can try to coordinate on getting #142 going.

cthoyt · 2022-07-22T16:50:51Z


-    def _get_response(self, path: str, *args, raise_for_status=True, **kwargs):
+    def _get_response(self, *args, **kwargs):
        check_limit()


do you know if the rate limit is the same for all instances? we could also push the limit checking upstream

It's basically a global rate limiter: https://github.com/INCATools/ontology-access-kit/blob/main/src/oaklib/utilities/rate_limiter.py which seems perhaps too conservative, but there may be some good reasons for it that I'm not aware of.

By instances, I mean bioportal, agoportal, etc. I know BioPortal says 15 requests per second on https://www.bioontology.org/wiki/Annotator_Optimizing_and_Troublehooting, but finding this kind of specific documentation for all portals is annoying

Ah I see, no I don't know off hand if all ontoportal instances have the same rate limits or if it's configurable.

cmungall · 2022-07-22T19:12:23Z

Yes, consider this a first pass. Globally it's conservative as some of the individual queries launched at the triplestore endpoints can be mega-queries, but it could be tuned for each category of endpoint, then by endpoint specifics

…

On Fri, Jul 22, 2022 at 10:07 AM Patrick Kalita ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/oaklib/implementations/ontoportal/ontoportal_implementation_base.py <#204 (comment)> : > def prefix_map(self) -> PREFIX_MAP: # TODO return {} - def _get_response(self, path: str, *args, raise_for_status=True, **kwargs): + def _get_response(self, *args, **kwargs): check_limit() It's basically a global rate limiter: https://github.com/INCATools/ontology-access-kit/blob/main/src/oaklib/utilities/rate_limiter.py which seems perhaps too conservative, but there may be some good reasons for it that I'm not aware of. — Reply to this email directly, view it on GitHub <#204 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAMMOKZKLQMO4MWAN6R37TVVLIMDANCNFSM54JDSJRQ> . You are receiving this because your review was requested.Message ID: ***@***.***>

cmungall · 2022-07-22T19:56:26Z

I think using pystow universally for api keys is good. Up to you both how best to go about it ie merge other pr, redo work on this one

…

On Fri, Jul 22, 2022 at 12:40 PM Charles Tapley Hoyt < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/oaklib/implementations/ontoportal/ontoportal_implementation_base.py <#204 (comment)> : > @@ -50,22 +49,18 @@ def __post_init__(self): if self.focus_ontology is None: if self.resource: self.focus_ontology = self.resource.slug - if not self.OntoPortalClientClass: - raise NotImplementedError("OntoPortalClientClass not specified") - api_key = get_apikey_value(self.api_key_name) - self.client = self.OntoPortalClientClass(api_key=api_key) + if not self.ontoportal_client_class: + raise NotImplementedError("ontoportal_client_class not specified") + api_key = get_apikey_value(self.ontoportal_client_class.name) In #142 <#142> I made it much more integrated with PyStow, since it's a pain to have to use several competing systems when there's already something ready to go out of the box. If someone wants to put effort into unmucking the testing (#173 <#173>), then it will be possible to finish that PR, since right now the logging is too much of a mess to even figure it out. If I had to guess though, it might be because PRs from forks don't get access to the secrets in the GHA workflows — Reply to this email directly, view it on GitHub <#204 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAMMOMOVKUNGBHMEY4E6WDVVL2MBANCNFSM54JDSJRQ> . You are receiving this because your review was requested.Message ID: ***@***.***>

pkalita-lbl marked this pull request as draft July 21, 2022 21:50

pkalita-lbl mentioned this pull request Jul 21, 2022

Externalize OntoPortal client functions (e.g., for BioPortal, AgroPortal, EcoPortal) #168

Open

Fix bioportal api key check in tests

8784314

pkalita-lbl marked this pull request as ready for review July 21, 2022 22:42

pkalita-lbl requested review from cmungall and cthoyt July 21, 2022 22:42

cmungall approved these changes Jul 21, 2022

View reviewed changes

Rename OntoPortalInterface to OntoPortalImplementationBase

209816c

Fix lint errors

cecdf25

pkalita-lbl mentioned this pull request Jul 22, 2022

Allow BioPortal tests to run in GitHub workflow #205

Closed

cthoyt reviewed Jul 22, 2022

View reviewed changes

Comment thread src/oaklib/implementations/bioportal/agroportal_implementation.py Outdated

cthoyt reviewed Jul 22, 2022

View reviewed changes

Comment thread src/oaklib/implementations/bioportal/agroportal_implementation.py Outdated

cthoyt reviewed Jul 22, 2022

View reviewed changes

Upgrade to ontoportal-client v0.0.3

c208089

cthoyt reviewed Jul 22, 2022

View reviewed changes

pkalita-lbl merged commit 0d9f355 into main Jul 22, 2022

pkalita-lbl deleted the issue-168 branch July 22, 2022 23:27

cthoyt mentioned this pull request Sep 9, 2022

Externalize OLS implementation #270

Merged

Conversation

pkalita-lbl commented Jul 21, 2022

Uh oh!

cmungall left a comment

Choose a reason for hiding this comment

Uh oh!

pkalita-lbl commented Jul 22, 2022

Uh oh!

cthoyt commented Jul 22, 2022

Uh oh!

pkalita-lbl commented Jul 22, 2022

Uh oh!

cthoyt commented Jul 22, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmungall commented Jul 22, 2022 via email

Uh oh!

cmungall commented Jul 22, 2022 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants