INCATools / ontology-access-kit

Ontology Access Kit: A python library and command line application for working with ontologies
https://incatools.github.io/ontology-access-kit/
Apache License 2.0
109 stars 23 forks source link

Using `oakx-plugin` with `oaklib` dependency greater than version 0.1.53 gives an obscure error. #394

Open hrshdhgd opened 1 year ago

hrshdhgd commented 1 year ago

This problem arose when @justaddcoffee , @caufieldjh and I were hacking together on oakx-grape.

On using oaklib latest version, the command runoak -I sqlite:obo:pato terms returns IndexError: list index out of range . We debugged this deep into pkg_resources.

The code in oaklib where this error is thrown is here:

implementation_resolver.register_entrypoint("oaklib.plugins")

On digging deeper, it goes to the area where plugins are imported (using __import__). Exact snippet in pkg_resources would be:

def resolve(self):
        """
        Resolve the entry point from its module and attrs.
        """
        module = __import__(self.module_name, fromlist=['__name__'], level=0)
        try:
            return functools.reduce(getattr, self.attrs, module)
        except AttributeError as exc:
            raise ImportError(str(exc)) from exc

Something changed between v0.1.53 and v0.1.54 that may have caused this. Needs further investigation.

caufieldjh commented 1 year ago

I'm still suspicious about potential name collision with something in jupyter, and it looks like jupyter-core got a major version update to 5.0 as of this (0.1.54) OAK version

justaddcoffee commented 1 year ago

Possibly this is caused by a circular import? See here

hrshdhgd commented 1 year ago

Just documenting our slack conversation here:

One step closer. Commented everything out in grape_implementation.py with the latest version of oaklib. The command runoak -i sqlite:obo:pato terms works just fine.

Further

# from embiggen.edge_prediction.edge_prediction_ensmallen.perceptron import PerceptronEdgePrediction
# from embiggen.embedders.ensmallen_embedders.first_order_line import FirstOrderLINEEnsmallen
# from grape import Graph
# from grape.similarities import DAGResnik

Commented these imports and the rest of the code and the command works fine. Only uncommented non-grape and non-embiggen imports The moment I uncomment any of the imports above, the error reappears. Something worth noticing: When it does run, It first logs: WARNING:class_resolver.base:could not load grape which is because the import is commented. I don't know if grape plays a role in the error or no. Just documenting here

Harry Caufield:

That warning is thrown on an ImportError, I think, and a circular import will raise that error embiggen and oaklib share pandas as an import but I think that's it Unless this is due to an upstream import only introduced in oaklib 0.1.54

caufieldjh commented 1 year ago

Strangely enough, @hrshdhgd found that running runoak --stacktrace -i sqlite:obo:pato terms works as expected on OAK v0.1.63, but running the same with --vvv does not.

hrshdhgd commented 1 year ago

The problem code is oaklib is in cli.py(this line)

When I set sys.tracebacklimit = 1 , I get this stack trace:

Traceback (most recent call last):
  File "../oakx-grape/lib/python3.9/site-packages/embiggen/utils/abstract_models/auto_init.py", line 199, in build_init
    traceback.extract_stack()[-2].filename
IndexError: list index out of range

There are two solutions to this:

  1. Fix it in oaklib by setting it to 1 instead of 0 OR
  2. Fix it in embiggen.

cc: @cmungall @LucaCappelletti94

hrshdhgd commented 1 year ago

For now, we have implemented a band-aid fix until this issue is resolved.

LucaCappelletti94 commented 1 year ago

That's a fun one. I'll look into it!

LucaCappelletti94 commented 1 year ago

Question: why would you set sys.tracebacklimit = 1?

LucaCappelletti94 commented 1 year ago

Anyhow, I have now resolved it by using explicitly the default Python traceback limit when working in the meta-programming parts of GRAPE.

hrshdhgd commented 1 year ago

Question: why would you set sys.tracebacklimit = 1?

That was a band-aid fix for the time being. The stack would not be empty and thus the list index would not be out of range (when calling traceback.extract_stack()[-2]) thus letting us proceed.

LucaCappelletti94 commented 1 year ago

Sorry but I am not following. Why is there any stack trace manipulation?

hrshdhgd commented 1 year ago

It was inspired by this PR in linkml, implemented in this PR in oaklib. My guess: to avoid verbosity of the stack trace unless needed.

LucaCappelletti94 commented 1 year ago

While it is now fixed in grape, and I'll publish it soon, I must say that I find making errors less verbose a very bad call

cmungall commented 1 year ago

This is only set in the cli layer and our clis are used by a large number of non technical or semi technical people and the overwhelming consensus was to hide stack traces by default at the cli level. And this is following the tradition of our main OWL cli, robot

On Thu, Dec 15, 2022 at 6:18 AM Luca Cappelletti @.***> wrote:

While it is now fixed in grape, and I'll publish it soon, I must say that I find making errors less verbose a very bad call

— Reply to this email directly, view it on GitHub https://github.com/INCATools/ontology-access-kit/issues/394#issuecomment-1353166074, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAMMOKXAOIYMWIVSVK7YIDWNMSELANCNFSM6AAAAAASXHI7A4 . You are receiving this because you were mentioned.Message ID: @.***>

justaddcoffee commented 1 year ago

Is it feasible here to 1) by default hide stack traces without truncating them, and 2) write them out when the -v flag is set?

This was a pretty mysterious bug to track down, but 2) above would have made it a lot easier