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

fix: error in IPython while handling HTTP4xxClientError #486

Merged
merged 2 commits into from
Sep 11, 2022

Conversation

seeM
Copy link
Contributor

@seeM seeM commented Sep 10, 2022

For example:

In [1]: from ghapi.core import *

In [2]: GhApi().repos.get('org', 'not-a-real-repo-123123123')
Unexpected exception formatting exception. Falling back to standard exception
Traceback (most recent call last):
  File "/Users/seem/.pyenv/versions/3.9.13/lib/python3.9/site-packages/IPython/core/interactiveshell.py", line 3398, in run_code
    exec(code_obj, self.user_global_ns, self.user_ns)
  File "<ipython-input-2-b91955617868>", line 1, in <cell line: 1>
    GhApi().repos.get('org', 'not-a-real-repo-123123123')
  File "/Users/seem/code/ghapi/ghapi/core.py", line 61, in __call__
    return self.client(self.path, self.verb, headers=headers, route=route_p, query=query_p, data=data_p)
  File "/Users/seem/code/ghapi/ghapi/core.py", line 118, in __call__
    res,self.recv_hdrs = urlsend(path, verb, headers=headers or None, debug=debug, return_headers=True,
  File "/Users/seem/code/fastcore/fastcore/net.py", line 217, in urlsend
    return urlread(req, return_json=return_json, return_headers=return_headers)
  File "/Users/seem/code/fastcore/fastcore/net.py", line 118, in urlread
    if 400 <= e.code < 500: raise ExceptionsHTTP[e.code](e.url, e.hdrs, e.fp, msg=e.msg) from None
fastcore.basics.HTTP404NotFoundError: HTTP Error 404: Not Found
====Error Body====
{
  "message": "Not Found",
  "documentation_url": "https://docs.github.com/rest/reference/repos#get-a-repository"
}


During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/seem/.pyenv/versions/3.9.13/lib/python3.9/site-packages/IPython/core/interactiveshell.py", line 1990, in showtraceback
    if hasattr(value, "_render_traceback_"):
  File "/Users/seem/.pyenv/versions/3.9.13/lib/python3.9/tempfile.py", line 472, in __getattr__
    file = self.__dict__['file']
KeyError: 'file'

cc @hamelsmu @jph00

@seeM seeM added the bug Something isn't working label Sep 10, 2022
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

fastcore/net.py Outdated
Comment on lines 94 to 95
def _init(self, url, hdrs, fp, msg=msg, code=code): super(cls, self).__init__(url, code, msg, hdrs, fp)
cls.__init__ = _init
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The crux is to override the __init__ set by get_class to call super().__init__.

The _mk_error function was needed so that _init is included as-is in the closure. Without _mk_error, each iteration of the for loop would overwrite the previously defined _init.

Copy link
Member

Choose a reason for hiding this comment

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

This is getting pretty obscure! Is there any simpler way to do this, even if a bit more verbose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you like having separate classes for each error code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'd prefer to use the urllib.HTTPError class

Copy link
Member

Choose a reason for hiding this comment

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

The thing I do want is to be able to catch exceptions based on what kind of error occurred. Is there a way to do that without separate classes?

Copy link
Member

Choose a reason for hiding this comment

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

Would using type(name, bases, attributes) directly be an easier way to create these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think like so:

try: urlopen(...)
except HTTPError as e:
    if e.code != 401: raise
    # ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would using type(name, bases, attributes) directly be an easier way to create these?

I thought about it. type(nm, (HTTP4xxClientError,), {'code': code, 'msg': msg}) works and is concise, but would be a tiny breaking change in the signature, from current:

def __init__(self, url, hdrs, fp, msg=default_msg, code=default_code):

to the parent class':

def __init__(self, url, code, msg, hdrs, fp)

That might be okay though

Copy link
Member

Choose a reason for hiding this comment

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

Could you add __init__ to those attrs to get the init we want?

Comment on lines +94 to +95
def _init(self, url, hdrs, fp, msg=msg, code=code): HTTP4xxClientError.__init__(self, url, code, msg, hdrs, fp)
cls = type(nm, (HTTP4xxClientError,), {'__init__':_init})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jph00 I think this is much better with your suggestion to use type.

Another thing that simplifies it is to directly call HTTP4xxClientError.__init__ instead of super(...), because:

  1. Using super dynamically like this requires explicitly passing the cls. And in order to pass a different cls each time, we need to define _init in a closure to avoid overwriting it with each cls.
  2. Since _init needs a reference to cls, we have to first define it with type(...) and then patch cls.__init__.

This of course has the downside that HTTP4xxClientError is hardcoded instead of having super's behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Out of interest, this seems to work too, but I think it's more obscure:

for code,msg in _httperrors:
    class _(HTTP4xxClientError):
        def __init__(self, url, hdrs, fp, msg=msg, code=code): super().__init__(url, code, msg, hdrs, fp)
    _.__name__ = f'HTTP{code}{msg.replace(" ","")}Error'
    _.__qualname__ = _.__qualname__.split('.')[0] + _.__name__
    globals()[nm] = ExceptionsHTTP[code] = _

Copy link
Member

@jph00 jph00 Sep 11, 2022

Choose a reason for hiding this comment

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

I guess in that case I'd have a function like:

get _httperrcls(url, code, msg, hdrs, fp):
    class _c(HTTP4xxClientError):
            def __init__(self, url, hdrs, fp, msg=msg, code=code): super().__init__(url, code, msg, hdrs, fp)
     _c.__name__ = f'HTTP{code}{msg.replace(" ","")}Error'
     _c.__qualname__ = _.__qualname__.split('.')[0] + _c.__name__
    return _c

@jph00
Copy link
Member

jph00 commented Sep 11, 2022

Yay!

@jph00 jph00 merged commit 0ee5b7f into fastai:master Sep 11, 2022
@seeM seeM deleted the fix-http-exceptions branch September 11, 2022 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants