Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose intrinsics as decorated Python functions instead #20874

Merged
merged 1 commit into from
May 31, 2024

Conversation

stuhood
Copy link
Sponsor Member

@stuhood stuhood commented May 6, 2024

Remove intrinsics facility from the engine, and instead expose intrinsics in intrinsics.py via a new native call response for generators. This solves an issue in #19730 that there were no obvious @rule functions to call for intrinsics.

The native call response wraps a Rust future, which we trampoline to execute on the Rust runtime, and then return a value to the generator. It should work generically for any use case where we would like @rule code to await native code, which opens the door to fixing #11329 by making Workspace.write_digest async.

@stuhood stuhood added the category:internal CI, fixes for not-yet-released features, etc. label May 6, 2024
@stuhood stuhood force-pushed the stuhood.intrinsics-as-native-decorated branch 3 times, most recently from 487d30d to 1b7c9f1 Compare May 31, 2024 04:45
@stuhood stuhood marked this pull request as ready for review May 31, 2024 04:45
@stuhood stuhood force-pushed the stuhood.intrinsics-as-native-decorated branch from 1b7c9f1 to 7dbb37d Compare May 31, 2024 05:12
@stuhood stuhood force-pushed the stuhood.intrinsics-as-native-decorated branch from 7dbb37d to 33ff599 Compare May 31, 2024 05:14
@stuhood stuhood enabled auto-merge (squash) May 31, 2024 05:16
Copy link
Contributor

@huonw huonw left a comment

Choose a reason for hiding this comment

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

Nice. I think this looks good, but it's at the limit of my understanding.

Playing back, it looks like the PR has a few different moving parts:

  1. setup the PyGeneratorResponseNativeCall type to store a Rust future in a Python object
  2. support extracting and awaiting that future in Task::generate (plus GeneratorResponse::NativeCall and generator_send etc.)
  3. change how the intrinsics are defined and registered in Rust, which includes removing the Intrinsic and Intrinsics types and the Rule::Intrinsic handling
  4. rewrite all the intrinsics to:
    • query the thread-local task context since they no longer get it as an arg
    • wrap their internal future in PyGeneratorResponseNativeCall (which requires a few other adjustments, due to type inference, etc.?)
  5. define some normal @rules as wrappers

Then, because we've fully switched over in the definitions, I think this code is being actively exercised by the full test suite. (I'm raising this because: if this was instead a "preparation" that wasn't being used, we might discover a whole bunch of issues when we start using them. But, because they're being used directly by the old test-suite, that's a strong indication that things fit together and there's no behaviour change.)

Have I got that correct (or at least correct enough)?

Approved assuming I have.

@stuhood stuhood merged commit f8b6c42 into main May 31, 2024
25 checks passed
@stuhood stuhood deleted the stuhood.intrinsics-as-native-decorated branch May 31, 2024 07:48
@stuhood
Copy link
Sponsor Member Author

stuhood commented May 31, 2024

That is all exactly right. Thank you @huonw ! ... I apologize for such a skimpy PR description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants