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

Add method to export JIT Code from UOp Executor #32

Closed
wants to merge 1 commit into from

Conversation

tonybaloney
Copy link

I feel This is really important for testing and verification. Could come later.

@tonybaloney
Copy link
Author

"tonybaloney requested a review from gvanrossum"

GitHub did that automatically, I didn't request a review from Guido on his holiday :-)

@gvanrossum
Copy link
Collaborator

My holiday is now officially over. :-)

@brandtbucher
Copy link
Owner

Wow, codeowners are pinged on forks? That seems like a bug.

I think this PR makes sense... maybe it can be added in its own PR once the JIT is on main? I've really tried to strip this branch down to just the essential stuff (for reviewer focus).

Separately, I feel like valid and jit_code feel more like properties, not methods (since they're just fields on the executor). What do you think?

@tonybaloney
Copy link
Author

Wow, codeowners are pinged on forks? That seems like a bug.

It's a feature 😁 iirc there is a flag to disable that.

I think this PR makes sense... maybe it can be added in its own PR once the JIT is on main? I've really tried to strip this branch down to just the essential stuff (for reviewer focus).

Separately, I feel like valid and jit_code feel more like properties, not methods (since they're just fields on the executor). What do you think?

Agreed. You have your work cut out getting through all the comments on the main PR.
I'll raise this one later when the noise dies down.

@brandtbucher
Copy link
Owner

This is upstream now!

Sign up for free to 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.

3 participants