Exception fluency monkeypatching - #30

Merged
erikrose merged 4 commits into
mainfrom
exception-fluency-monkeypatching
Feb 3, 2026
Merged

Exception fluency monkeypatching#30
erikrose merged 4 commits into
mainfrom
exception-fluency-monkeypatching

Conversation

@erikrose

@erikroseerikrose commented Jan 16, 2026

Copy link
Copy Markdown
Member

This excises most of the Weird from our exception handling and leaves plenty of maneuvering space for future improvements. The 2 obvious ones are…

  1. Make an affordance for manual polish of individual exceptions, like adding nice getter properties, without clashing with the code generation. I have some ideas on how to do this nicely.
  2. Patch the docstrings of the wrapped routines so they say which new-style exceptions they :raise:. That'll make Sphinx or whatever tell the truth to our users.

But for now, this gets us most of the way there:

  • 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.
  • Port requests façade and the exception catch in wsgi.py to 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.

@erikroseerikrose mentioned this pull request Jan 28, 2026
@erikrose
erikroseforce-pushed the exception-fluency-monkeypatching branch 5 times, most recently from fa0df5f to 2b76077CompareJanuary 29, 2026 20:06
* 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.
@erikrose
erikrose marked this pull request as ready for review January 29, 2026 20:11
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.

@posborneposborne left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread.gitignore
# Generated code
/stubs/
__pycache__
/fastly_compute/exceptions/*

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 patchedfromwit_world.importsimportcompute_runtime# Using generated wrapper (details flexible)fromfastly_compute.witimportcompute_runtime

With 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.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suppose, add to the list of downsides with __init__.py monkeypatching.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: typo.

from .utils import indent, lower_snake, only, shouty_snake, upper_camel


class DocsHaver:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@erikrose

Copy link
Copy Markdown
MemberAuthor

I'll land this as-is so it unblocks rebasing #34 and #35 and then follow up with PRs that port test_nice_exceptions.py to Viceroy and commit the generated artifacts. Thanks!

@erikrose
erikrose merged commit 83038db into mainFeb 3, 2026
4 checks passed
@erikrose
erikrose deleted the exception-fluency-monkeypatching branch February 3, 2026 16:08
@erikroseerikrose mentioned this pull request Feb 17, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@erikrose@posborne
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Exception fluency monkeypatching - #30

Merged
erikrose merged 4 commits into
mainfrom
exception-fluency-monkeypatching
Feb 3, 2026
Merged

Exception fluency monkeypatching#30
erikrose merged 4 commits into
mainfrom
exception-fluency-monkeypatching

Conversation

@erikrose

@erikroseerikrose commented Jan 16, 2026

Copy link
Copy Markdown
Member

This excises most of the Weird from our exception handling and leaves plenty of maneuvering space for future improvements. The 2 obvious ones are…

  1. Make an affordance for manual polish of individual exceptions, like adding nice getter properties, without clashing with the code generation. I have some ideas on how to do this nicely.
  2. Patch the docstrings of the wrapped routines so they say which new-style exceptions they :raise:. That'll make Sphinx or whatever tell the truth to our users.

But for now, this gets us most of the way there:

  • 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.
  • Port requests façade and the exception catch in wsgi.py to 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.

@erikroseerikrose mentioned this pull request Jan 28, 2026
@erikrose
erikroseforce-pushed the exception-fluency-monkeypatching branch 5 times, most recently from fa0df5f to 2b76077CompareJanuary 29, 2026 20:06
* 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.
@erikrose
erikrose marked this pull request as ready for review January 29, 2026 20:11
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.

@posborneposborne left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread.gitignore
# Generated code
/stubs/
__pycache__
/fastly_compute/exceptions/*

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 patchedfromwit_world.importsimportcompute_runtime# Using generated wrapper (details flexible)fromfastly_compute.witimportcompute_runtime

With 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.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suppose, add to the list of downsides with __init__.py monkeypatching.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: typo.

from .utils import indent, lower_snake, only, shouty_snake, upper_camel


class DocsHaver:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@erikrose

Copy link
Copy Markdown
MemberAuthor

I'll land this as-is so it unblocks rebasing #34 and #35 and then follow up with PRs that port test_nice_exceptions.py to Viceroy and commit the generated artifacts. Thanks!

@erikrose
erikrose merged commit 83038db into mainFeb 3, 2026
4 checks passed
@erikrose
erikrose deleted the exception-fluency-monkeypatching branch February 3, 2026 16:08
@erikroseerikrose mentioned this pull request Feb 17, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@erikrose@posborne
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Exception fluency monkeypatching - #30

Merged
erikrose merged 4 commits into
mainfrom
exception-fluency-monkeypatching
Feb 3, 2026
Merged

Exception fluency monkeypatching#30
erikrose merged 4 commits into
mainfrom
exception-fluency-monkeypatching

Conversation

@erikrose

@erikroseerikrose commented Jan 16, 2026

Copy link
Copy Markdown
Member

This excises most of the Weird from our exception handling and leaves plenty of maneuvering space for future improvements. The 2 obvious ones are…

  1. Make an affordance for manual polish of individual exceptions, like adding nice getter properties, without clashing with the code generation. I have some ideas on how to do this nicely.
  2. Patch the docstrings of the wrapped routines so they say which new-style exceptions they :raise:. That'll make Sphinx or whatever tell the truth to our users.

But for now, this gets us most of the way there:

  • 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.
  • Port requests façade and the exception catch in wsgi.py to 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.

@erikroseerikrose mentioned this pull request Jan 28, 2026
@erikrose
erikroseforce-pushed the exception-fluency-monkeypatching branch 5 times, most recently from fa0df5f to 2b76077CompareJanuary 29, 2026 20:06
* 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.
@erikrose
erikrose marked this pull request as ready for review January 29, 2026 20:11
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.

@posborneposborne left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread.gitignore
# Generated code
/stubs/
__pycache__
/fastly_compute/exceptions/*

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 patchedfromwit_world.importsimportcompute_runtime# Using generated wrapper (details flexible)fromfastly_compute.witimportcompute_runtime

With 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.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suppose, add to the list of downsides with __init__.py monkeypatching.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: typo.

from .utils import indent, lower_snake, only, shouty_snake, upper_camel


class DocsHaver:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@erikrose

Copy link
Copy Markdown
MemberAuthor

I'll land this as-is so it unblocks rebasing #34 and #35 and then follow up with PRs that port test_nice_exceptions.py to Viceroy and commit the generated artifacts. Thanks!

@erikrose
erikrose merged commit 83038db into mainFeb 3, 2026
4 checks passed
@erikrose
erikrose deleted the exception-fluency-monkeypatching branch February 3, 2026 16:08
@erikroseerikrose mentioned this pull request Feb 17, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@erikrose@posborne
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Exception fluency monkeypatching - #30

Merged
erikrose merged 4 commits into
mainfrom
exception-fluency-monkeypatching
Feb 3, 2026
Merged

Exception fluency monkeypatching#30
erikrose merged 4 commits into
mainfrom
exception-fluency-monkeypatching

Conversation

@erikrose

@erikroseerikrose commented Jan 16, 2026

Copy link
Copy Markdown
Member

This excises most of the Weird from our exception handling and leaves plenty of maneuvering space for future improvements. The 2 obvious ones are…

  1. Make an affordance for manual polish of individual exceptions, like adding nice getter properties, without clashing with the code generation. I have some ideas on how to do this nicely.
  2. Patch the docstrings of the wrapped routines so they say which new-style exceptions they :raise:. That'll make Sphinx or whatever tell the truth to our users.

But for now, this gets us most of the way there:

  • 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.
  • Port requests façade and the exception catch in wsgi.py to 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.

@erikroseerikrose mentioned this pull request Jan 28, 2026
@erikrose
erikroseforce-pushed the exception-fluency-monkeypatching branch 5 times, most recently from fa0df5f to 2b76077CompareJanuary 29, 2026 20:06
* 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.
@erikrose
erikrose marked this pull request as ready for review January 29, 2026 20:11
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.

@posborneposborne left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread.gitignore
# Generated code
/stubs/
__pycache__
/fastly_compute/exceptions/*

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 patchedfromwit_world.importsimportcompute_runtime# Using generated wrapper (details flexible)fromfastly_compute.witimportcompute_runtime

With 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.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suppose, add to the list of downsides with __init__.py monkeypatching.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: typo.

from .utils import indent, lower_snake, only, shouty_snake, upper_camel


class DocsHaver:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@erikrose

Copy link
Copy Markdown
MemberAuthor

I'll land this as-is so it unblocks rebasing #34 and #35 and then follow up with PRs that port test_nice_exceptions.py to Viceroy and commit the generated artifacts. Thanks!

@erikrose
erikrose merged commit 83038db into mainFeb 3, 2026
4 checks passed
@erikrose
erikrose deleted the exception-fluency-monkeypatching branch February 3, 2026 16:08
@erikroseerikrose mentioned this pull request Feb 17, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@erikrose@posborne
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Exception fluency monkeypatching - #30

Merged
erikrose merged 4 commits into
mainfrom
exception-fluency-monkeypatching
Feb 3, 2026
Merged

Exception fluency monkeypatching#30
erikrose merged 4 commits into
mainfrom
exception-fluency-monkeypatching

Conversation

@erikrose

@erikroseerikrose commented Jan 16, 2026

Copy link
Copy Markdown
Member

This excises most of the Weird from our exception handling and leaves plenty of maneuvering space for future improvements. The 2 obvious ones are…

  1. Make an affordance for manual polish of individual exceptions, like adding nice getter properties, without clashing with the code generation. I have some ideas on how to do this nicely.
  2. Patch the docstrings of the wrapped routines so they say which new-style exceptions they :raise:. That'll make Sphinx or whatever tell the truth to our users.

But for now, this gets us most of the way there:

  • 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.
  • Port requests façade and the exception catch in wsgi.py to 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.

@erikroseerikrose mentioned this pull request Jan 28, 2026
@erikrose
erikroseforce-pushed the exception-fluency-monkeypatching branch 5 times, most recently from fa0df5f to 2b76077CompareJanuary 29, 2026 20:06
* 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.
@erikrose
erikrose marked this pull request as ready for review January 29, 2026 20:11
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.

@posborneposborne left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread.gitignore
# Generated code
/stubs/
__pycache__
/fastly_compute/exceptions/*

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 patchedfromwit_world.importsimportcompute_runtime# Using generated wrapper (details flexible)fromfastly_compute.witimportcompute_runtime

With 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.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suppose, add to the list of downsides with __init__.py monkeypatching.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: typo.

from .utils import indent, lower_snake, only, shouty_snake, upper_camel


class DocsHaver:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@erikrose

Copy link
Copy Markdown
MemberAuthor

I'll land this as-is so it unblocks rebasing #34 and #35 and then follow up with PRs that port test_nice_exceptions.py to Viceroy and commit the generated artifacts. Thanks!

@erikrose
erikrose merged commit 83038db into mainFeb 3, 2026
4 checks passed
@erikrose
erikrose deleted the exception-fluency-monkeypatching branch February 3, 2026 16:08
@erikroseerikrose mentioned this pull request Feb 17, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@erikrose@posborne
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Exception fluency monkeypatching - #30

Merged
erikrose merged 4 commits into
mainfrom
exception-fluency-monkeypatching
Feb 3, 2026
Merged

Exception fluency monkeypatching#30
erikrose merged 4 commits into
mainfrom
exception-fluency-monkeypatching

Conversation

@erikrose

@erikroseerikrose commented Jan 16, 2026

Copy link
Copy Markdown
Member

This excises most of the Weird from our exception handling and leaves plenty of maneuvering space for future improvements. The 2 obvious ones are…

  1. Make an affordance for manual polish of individual exceptions, like adding nice getter properties, without clashing with the code generation. I have some ideas on how to do this nicely.
  2. Patch the docstrings of the wrapped routines so they say which new-style exceptions they :raise:. That'll make Sphinx or whatever tell the truth to our users.

But for now, this gets us most of the way there:

  • 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.
  • Port requests façade and the exception catch in wsgi.py to 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.

@erikroseerikrose mentioned this pull request Jan 28, 2026
@erikrose
erikroseforce-pushed the exception-fluency-monkeypatching branch 5 times, most recently from fa0df5f to 2b76077CompareJanuary 29, 2026 20:06
* 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.
@erikrose
erikrose marked this pull request as ready for review January 29, 2026 20:11
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.

@posborneposborne left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread.gitignore
# Generated code
/stubs/
__pycache__
/fastly_compute/exceptions/*

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 patchedfromwit_world.importsimportcompute_runtime# Using generated wrapper (details flexible)fromfastly_compute.witimportcompute_runtime

With 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.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suppose, add to the list of downsides with __init__.py monkeypatching.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: typo.

from .utils import indent, lower_snake, only, shouty_snake, upper_camel


class DocsHaver:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@erikrose

Copy link
Copy Markdown
MemberAuthor

I'll land this as-is so it unblocks rebasing #34 and #35 and then follow up with PRs that port test_nice_exceptions.py to Viceroy and commit the generated artifacts. Thanks!

@erikrose
erikrose merged commit 83038db into mainFeb 3, 2026
4 checks passed
@erikrose
erikrose deleted the exception-fluency-monkeypatching branch February 3, 2026 16:08
@erikroseerikrose mentioned this pull request Feb 17, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@erikrose@posborne
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Exception fluency monkeypatching - #30

Merged
erikrose merged 4 commits into
mainfrom
exception-fluency-monkeypatching
Feb 3, 2026
Merged

Exception fluency monkeypatching#30
erikrose merged 4 commits into
mainfrom
exception-fluency-monkeypatching

Conversation

@erikrose

@erikroseerikrose commented Jan 16, 2026

Copy link
Copy Markdown
Member

This excises most of the Weird from our exception handling and leaves plenty of maneuvering space for future improvements. The 2 obvious ones are…

  1. Make an affordance for manual polish of individual exceptions, like adding nice getter properties, without clashing with the code generation. I have some ideas on how to do this nicely.
  2. Patch the docstrings of the wrapped routines so they say which new-style exceptions they :raise:. That'll make Sphinx or whatever tell the truth to our users.

But for now, this gets us most of the way there:

  • 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.
  • Port requests façade and the exception catch in wsgi.py to 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.

@erikroseerikrose mentioned this pull request Jan 28, 2026
@erikrose
erikroseforce-pushed the exception-fluency-monkeypatching branch 5 times, most recently from fa0df5f to 2b76077CompareJanuary 29, 2026 20:06
* 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.
@erikrose
erikrose marked this pull request as ready for review January 29, 2026 20:11
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.

@posborneposborne left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread.gitignore
# Generated code
/stubs/
__pycache__
/fastly_compute/exceptions/*

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 patchedfromwit_world.importsimportcompute_runtime# Using generated wrapper (details flexible)fromfastly_compute.witimportcompute_runtime

With 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.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suppose, add to the list of downsides with __init__.py monkeypatching.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: typo.

from .utils import indent, lower_snake, only, shouty_snake, upper_camel


class DocsHaver:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@erikrose

Copy link
Copy Markdown
MemberAuthor

I'll land this as-is so it unblocks rebasing #34 and #35 and then follow up with PRs that port test_nice_exceptions.py to Viceroy and commit the generated artifacts. Thanks!

@erikrose
erikrose merged commit 83038db into mainFeb 3, 2026
4 checks passed
@erikrose
erikrose deleted the exception-fluency-monkeypatching branch February 3, 2026 16:08
@erikroseerikrose mentioned this pull request Feb 17, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@erikrose@posborne
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Exception fluency monkeypatching - #30

Merged
erikrose merged 4 commits into
mainfrom
exception-fluency-monkeypatching
Feb 3, 2026
Merged

Exception fluency monkeypatching#30
erikrose merged 4 commits into
mainfrom
exception-fluency-monkeypatching

Conversation

@erikrose

@erikroseerikrose commented Jan 16, 2026

Copy link
Copy Markdown
Member

This excises most of the Weird from our exception handling and leaves plenty of maneuvering space for future improvements. The 2 obvious ones are…

  1. Make an affordance for manual polish of individual exceptions, like adding nice getter properties, without clashing with the code generation. I have some ideas on how to do this nicely.
  2. Patch the docstrings of the wrapped routines so they say which new-style exceptions they :raise:. That'll make Sphinx or whatever tell the truth to our users.

But for now, this gets us most of the way there:

  • 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.
  • Port requests façade and the exception catch in wsgi.py to 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.

@erikroseerikrose mentioned this pull request Jan 28, 2026
@erikrose
erikroseforce-pushed the exception-fluency-monkeypatching branch 5 times, most recently from fa0df5f to 2b76077CompareJanuary 29, 2026 20:06
* 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.
@erikrose
erikrose marked this pull request as ready for review January 29, 2026 20:11
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.

@posborneposborne left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread.gitignore
# Generated code
/stubs/
__pycache__
/fastly_compute/exceptions/*

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 patchedfromwit_world.importsimportcompute_runtime# Using generated wrapper (details flexible)fromfastly_compute.witimportcompute_runtime

With 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.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suppose, add to the list of downsides with __init__.py monkeypatching.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: typo.

from .utils import indent, lower_snake, only, shouty_snake, upper_camel


class DocsHaver:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@erikrose

Copy link
Copy Markdown
MemberAuthor

I'll land this as-is so it unblocks rebasing #34 and #35 and then follow up with PRs that port test_nice_exceptions.py to Viceroy and commit the generated artifacts. Thanks!

@erikrose
erikrose merged commit 83038db into mainFeb 3, 2026
4 checks passed
@erikrose
erikrose deleted the exception-fluency-monkeypatching branch February 3, 2026 16:08
@erikroseerikrose mentioned this pull request Feb 17, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@erikrose@posborne