Exception fluency monkeypatching#30
Conversation
erikrose
force-pushed
the
exception-fluency-monkeypatching
branch
5 times, most recently
from
fa0df5f to
2b76077
Compare
* Based on a union of all the `result` error types from the WIT, generate more specific exception classes. * Generate patches to make componentize-py-generated routines raise those exceptions. * Apply those monkeypatches at `fastly_compute` import time. * Move `remap_wit_error()` to its new home right next to the monkeypatcher which should be its exclusive caller. * Teach makefile to run the code generator at the right times.
Mostly, don't crash trying to monkeypatch nonexistent hostcalls when imported by the testrunner in the absence of Viceroy. Can't patch hostcalls in the absence of a host!
This is a quick one-to-one port to get things running. Something we should consider before release is to map ErrorWithDetail to one of a set of exceptions corresponding to the detail enumeration if present, otherwise falling back to the error itself. It's not important to the `requests` package since it just turns around and does its own mapping to a `requests`-style exception hierarchy, but it'd be a big ergonomic win for normal callers.
I think it's better to emphasize that we're patching at runtime. Are we patching the WIT? Not really; we're patching stuff generated from the WIT. This is less misleading.
posborne
left a comment
posborne
left a comment
There was a problem hiding this comment.
I'm fine with moving forward with the change and evolving as we go and don't want to get too caught up bikeshedding. I do dislike the monkeypatching but also don't want to stop progress over it.
| # Generated code | ||
| /stubs/ | ||
| __pycache__ | ||
| /fastly_compute/exceptions/* |
There was a problem hiding this comment.
I would probably be OK and it may make for easier reference to include this generated code in tree given that the inputs probably don't change too frequently and we probably do want to examine changes that occur fairly closely.
When to do regen then becomes a bit different, potentially; perhaps do it manually with some kind of CI check to catch it containing non-cosmetic differences. Not a hill I'd die on, but I do think this could be a case where including the generated code might be worthwhile to aid reference.
There was a problem hiding this comment.
100% agree. I was wishing for that as I developed it, so I'll open a follow-up PR with that change. It's a fairly elegant way of putting this code under test.
| sys.path.append(str(Path(__file__).parent.parent / "stubs")) | ||
|
|
||
| from componentize_py_types import Err | ||
| from wit_world.imports.types import Error_BufferLen, OpenError |
There was a problem hiding this comment.
I'm running into issues with running tests locally; in general, I think we should avoid importing anything from stubs/wit_world/componentize_py_types in the host python environment as a rule.
We can probably have this test but I think it may be less problematic (though slightly annoying) to move it to exist behind a test shim so it runs within the guest.
|
|
||
| # Before anything from the fastly_compute package is used, do our monkeypatching | ||
| # to make the WIT-generated code act more Pythonically: | ||
| patch() |
There was a problem hiding this comment.
I'm still uneasy about doing this kind of runtime patching. It introduces magic that is not necessary without any benefit other than reducing the amount of code that needs to be generated slightly.
The alternative I still believe is preferred is to have a generated layer that does the translation; this is similar to what we are patching in at runtime but generated a compile time. Use would look like this:
# Current, use wit_world import that gets magically patched
from wit_world.imports import compute_runtime
# Using generated wrapper (details flexible)
from fastly_compute.wit import compute_runtimeWith that in mind, I also think we can move forward with the change as-is and it will not preclude us from changing our approach on this later on. Both would perform a similar transform, sharing most of the same code.
I'll keep thinking on this, but wanted to speak my peace that this feels like a bit of a hack that could cause us some pain and it isn't necessary to achieve what is being achieved here.
There was a problem hiding this comment.
Noted! I'm not ordinarily a monkeypatching kind of guy either, but I think in this case the good outweighs the bad. My motivating good is that it effectively clears the undesirable behavior out of the system; no one is going to call a low-level Err-raising routine by accident. That wouldn't ordinarily be worth much worry, except that many of our routines are methods on resources (on classes, in the Python world). Any source-level wrapping would have to carry around a duplicate of each class to house nice-exception-raising methods. Now you've got 2 different Request classes kicking around, throwing different exceptions, returning different kinds of other classes. One slip by a customer or a lib they use (should we be so successful), and you could end up with fairly subtle bugs involving same-named classes, ones which might not be discovered except under (less-tested) error conditions.
A halfway approach might be to throw some kind of warning if wit_world is imported directly. Or stow it under its_a_terrible_idea_to_import_this.wit_world etc.
It might not be a bad idea to convert this into a "2-way door" by importing everything in wit_world through to fastly_compute.wit after all (resurrecting #32). I'm 3/5 in favor of this. What do you think?
| " # Tolerate that momentary import for the testrunner before Viceroy, and thus\n" | ||
| " # the wit_world, is around.\n" | ||
| " def patch():\n" | ||
| ' print("Faking the run of exception-mapping monkeypatches for test runner.")\n' |
There was a problem hiding this comment.
I suppose, add to the list of downsides with __init__.py monkeypatching.
There was a problem hiding this comment.
Yes, this is a silliness. I'm inclined to port test_nice_exceptions.py to run under Viceroy, which would nix this. It also gets rid of an icky global sys.path twiddle it currently does. I'm uncomfortable about consequences of that leaking out to other test code.
|
|
||
| In practice, many types, like variants and the unit type, are represented by | ||
| more-specific subclasses, leaving this one to stand in for ones we haven't | ||
| needed to specializze for yet. |
| from .utils import indent, lower_snake, only, shouty_snake, upper_camel | ||
|
|
||
|
|
||
| class DocsHaver: |
There was a problem hiding this comment.
Consider having the abc have a constructor that assigns _me rather than having that be a protocol? I don't care that much, just seems more explicit to have it handled via a chain of super constructors if we're going to have the inheritance hierarchy.
2 participants
This excises most of the Weird from our exception handling and leaves plenty of maneuvering space for future improvements. The 2 obvious ones are…
:raise:. That'll make Sphinx or whatever tell the truth to our users.But for now, this gets us most of the way there:
resulterror types from the WIT, generate more specific exception classes.fastly_computeimport time.remap_wit_error()to its new home right next to the monkeypatcher which should be its exclusive caller.requestsfaçade and the exception catch inwsgi.pyto the new-style exceptions.For the moment, I've left the make-tests-passing commit separate just in case you can think of a nicer way of doing it, like perhaps running the patches at sometime other than import time.