FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-38500: Add _PyInterpreterState_SetEvalFrameFunc() by vstinner · Pull Request #17340 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

bpo-38500: Add _PyInterpreterState_SetEvalFrameFunc() - #17340

Merged
vstinner merged 1 commit into
python:masterfrom
vstinner:set_eval_frame
Mar 12, 2020
ConversationCommits (1)ChecksFiles changed
Merged

bpo-38500: Add _PyInterpreterState_SetEvalFrameFunc()#17340
vstinner merged 1 commit into
python:masterfrom
vstinner:set_eval_frame

Conversation

vstinner commented Nov 22, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

PyInterpreterState.eval_frame function now requires a tstate (Python
thread state) parameter.

Add private functions to the C API to get and set the frame
evaluation function:

  • Add tstate parameter to _PyFrameEvalFunction function type.
  • Add _PyInterpreterState_GetEvalFrameFunc() and
    _PyInterpreterState_SetEvalFrameFunc() functions.
  • Add tstate parameter to _PyEval_EvalFrameDefault().

https://bugs.python.org/issue38500

vstinner mentioned this pull request Nov 22, 2019
Closed

brettcannon commented Nov 22, 2019

Copy link
Copy Markdown
Member

I'm holding off on looking at this until the tstate question is resolved.

ericsnowcurrently approved these changes Nov 22, 2019
View reviewed changes

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The change itself LGTM. However, I'm still not convinced that this is functionality we want to expose through the public API. So that question should be resolved before merging this. :)

(@brettcannon The question of passing tstate to the eval function seems unrelated.)

ericsnowcurrently commented Nov 22, 2019

Copy link
Copy Markdown
Member

Also, it would probably make sense to have an explicit mention of this in whatsnew doc (porting section?).

brettcannon commented Nov 22, 2019

Copy link
Copy Markdown
Member

@ericsnowcurrently I think the tstate discussion is related because @vstinner discussed changing the definition of PyFrameEvalFunction which is being explicitly dealt with here.

brettcannon removed their request for review January 22, 2020 23:46

vstinner commented Mar 10, 2020

Copy link
Copy Markdown
Member Author

Python 3.8.0 is now released. The internal C API can be used to access interp->eval_frame. I no longer want to backport this change to Python 3.8.

vstinner mentioned this pull request Mar 10, 2020
Closed

vstinner commented Mar 10, 2020 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member Author

I rebased my PR on master. I merged PR #17187 ("bpo-38818: PyInterpreterState.eval_frame now pass tstate #17187") changes into this PR.

vstinner commented Mar 10, 2020

Copy link
Copy Markdown
Member Author

I plan to merge my PR 17340 at the end of week to not miss Python 3.9 feature freeze deadline, unless someone speaks up and find a good reason to not merge this PR. The PR adds a public C API PyInterpreterState_SetEvalFrameFunc() and now pass tstate to the frame evaluation function.

Please go to https://bugs.python.org/issue38500 for discussion, don't discuss the topic here.

vstinner commented Mar 10, 2020

Copy link
Copy Markdown
Member Author

@DinoV @ericsnowcurrently @brettcannon: Would you mind to review the update PR?

vstinner commented Mar 10, 2020

Copy link
Copy Markdown
Member Author

The Windows ARM job of Azure Pipelines PR failed on downloading dependencies ("urllib.error.HTTPError: HTTP Error 401: UNAUTHORIZED"). I will wait for reviews before restarting the job. Right now, I don't need a green CI, I don't plan to merge this PR before the end of the week.

vstinner commented Mar 10, 2020

Copy link
Copy Markdown
Member Author

@rsumner868: Hi, I saw you approving a few PRs. Would you mind to elaborate what you reviewed and explain why/how you approve a PR? This PR is controversial, you cannot simply approve it with no message.

brettcannon self-requested a review March 10, 2020 19:57

markshannon commented Mar 12, 2020

Copy link
Copy Markdown
Member

As I've reiterated in https://bugs.python.org/issue38500, I am strongly opposed to this being merged.

vstinner changed the title bpo-38500: Add PyInterpreterState_SetEvalFrameFunc() bpo-38500: Add _PyInterpreterState_SetEvalFrameFunc() Mar 12, 2020
bpo-38500: Add _PyInterpreterState_SetEvalFrameFunc()
163244d 
PyInterpreterState.eval_frame function now requires a tstate (Python
thread state) parameter.

Add private functions to the C API to get and set the frame
evaluation function:

* Add tstate parameter to _PyFrameEvalFunction function type.
* Add _PyInterpreterState_GetEvalFrameFunc() and
  _PyInterpreterState_SetEvalFrameFunc() functions.
* Add tstate parameter to _PyEval_EvalFrameDefault().

vstinner commented Mar 12, 2020

Copy link
Copy Markdown
Member Author

I took @markshannon's concerns in account and I updated my PR to only add private functions to the C API, not public functions.

vstinner merged commit 0b72b23 into python:master Mar 12, 2020
vstinner deleted the set_eval_frame branch March 12, 2020 22:18

markshannon commented Mar 14, 2020

Copy link
Copy Markdown
Member

@vstinner Thank you for making these functions private.

However, if they are documented in the Doc/c-api/init.rst, then they aren't that private.
Could you remove the documentation?
These functions have the potential to hurt performance by a significant amount, so hiding them (or warning that might well be removed in 3.10) would be good.

vstinner commented Mar 14, 2020

Copy link
Copy Markdown
Member Author

However, if they are documented in the Doc/c-api/init.rst, then they aren't that private. Could you remove the documentation?

I prefer to document them. The feature is part of the PEP 523 and we already had this discussion :-)

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ericsnowcurrently ericsnowcurrently approved these changes
markshannon Awaiting requested review from markshannon
brettcannon Awaiting requested review from brettcannon

Assignees

No one assigned

Labels

DO-NOT-MERGE

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL