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.

Sign up for GitHub

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

Jump to bottom

Can PyModule::from_code enforce a non-empty filename? #4769

Open
sxlijin opened this issue Dec 5, 2024 · 3 comments · May be fixed by #4777
Open

Can PyModule::from_code enforce a non-empty filename? #4769

sxlijin opened this issue Dec 5, 2024 · 3 comments · May be fixed by #4777
Labels
confusing-api

Comments

@sxlijin
Copy link

sxlijin commented Dec 5, 2024

We ran into an issue recently where our pyo3-based library was causing user code to hang, and it took me a full day and a half to track it down. Once we were able to identify the problem (it was hidden behind a few layers of abstraction - unhandled error in a background thread and then some, see BoundaryML/baml#1214), we realized that the root cause of the issue was that in our pyo3 code, we had a PyModule::from_code(..., file_name = "", ...) call.

It turns out that when a user calls inspect.stack() in a Python context, where one of the stack frames references a variable created in this way, inspect.stack() will actually throw because empty filenames (understandably) are errors.

I wonder if PyModule::from_code should just always return PyErr in this scenario.

Pros:

  • safer out-of-the-box behavior (user code is more likely to be compatible with inspect)

Cons:

  • would a user ever want to deliberately set the filename to ""?
  • is this too much of a backwards-breaking change? since this would cause runtime errors, not build errors on pyo3 upgrade
@LilyFoote
Copy link
Contributor

I think it would be good to do something here. If we can't do a hard error because of backwards compatibility, I think we should at least warn that this is unlikely to be what is wanted.

@davidhewitt
Copy link
Member

One option instead of a hard error is that if file_name is empty we substitute it with some sensible default like <string>, which exec uses, for example. We could document that we do this.

Similarly we could default empty module_name to <module>?

>>> exec('raise ValueError()')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<string>", line 1, in <module>
ValueError

@sxlijin
Copy link
Author

sxlijin commented Dec 6, 2024

I think defaulting file_name makes sense; I'm not totally sure module_name should get defaulted. While I was testing it, inspect.stack() was perfectly happy with an empty string for module_name, so I think barring a clear reason to provide a default for module_name it should stay unset.

It also doesn't appear to be consistently set by exec:

>>> globals = {}
>>> m = exec('def foo():\n  pass', globals)
>>> repr(globals['foo'].__module__)
'None'

@sxlijin sxlijin linked a pull request Dec 6, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
confusing-api
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(pymodule): improve safety of PyModule::from_code sxlijin/pyo3
3 participants
@sxlijin @LilyFoote @davidhewitt