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 PyEval_EvalCodeEx e.g. as Python::run_ex #2281

Open
chris-laplante opened this issue Apr 6, 2022 · 11 comments
Open

Expose PyEval_EvalCodeEx e.g. as Python::run_ex #2281

chris-laplante opened this issue Apr 6, 2022 · 11 comments

Comments

@chris-laplante
Copy link
Contributor

chris-laplante commented Apr 6, 2022

I have a use-case where I need to evaluate some code and set args. It would be possible by exposing PyEval_EvalCodeEx e.g. as a Python::run_ex method. Or perhaps call it run_with_args?

I am trying to implement it myself for contribution to pyo3, but I am a little stuck on the correct method signature and type conversions. Here is what I have so far. It is mostly just copied from Python::run_code:

fn run_code_ex(
    self,
    code: &str,
    start: c_int,
    globals: Option<&PyDict>,
    locals: Option<&PyDict>,
    args: Option<&PyTuple>,
    kws: Option<&PyDict>,
    defs: Option<&PyTuple>,
    kwdefs: Option<&PyDict>,
    closure: Option<&PyTuple> // TODO no idea what this is. docs say 'a closure tuple of cells.'
) -> PyResult<&'py PyAny> {
    let code = CString::new(code)?;
    unsafe {
        let mptr = ffi::PyImport_AddModule("__main__\0".as_ptr() as *const _);
        if mptr.is_null() {
            return Err(PyErr::fetch(self));
        }

        let globals = globals
            .map(AsPyPointer::as_ptr)
            .unwrap_or_else(|| ffi::PyModule_GetDict(mptr));
        let locals = locals.map(AsPyPointer::as_ptr).unwrap_or(globals);

        let args = args.map(AsPyPointer::as_ptr).unwrap_or(std::ptr::null_mut());

        let code_obj = ffi::Py_CompileString(code.as_ptr(), "<string>\0".as_ptr() as _, start);
        if code_obj.is_null() {
            return Err(PyErr::fetch(self));
        }
        let res_ptr = ffi::PyEval_EvalCodeEx(
            code_obj,
            globals,
            locals,
            args, // ERROR: expecting PyObject *const *args
            0, // TODO
            std::ptr::null_mut(), // TODO
            0, // TODO
            std::ptr::null_mut(), // TODO
            0, // TODO
            std::ptr::null_mut(), // TODO
            std::ptr::null_mut(), // TODO
        );
        ffi::Py_DECREF(code_obj);

        self.from_owned_ptr_or_err(res_ptr)
    }
}

The error at the args line looks like this:

    = note: expected raw pointer `*mut *mut pyo3_ffi::PyObject`
               found raw pointer `*mut pyo3_ffi::PyObject`

It seems like I am passing a pointer to the tuple object, but the method is expecting an array of the tuple elements?

A bit of guidance would be appreciated :)

@mejrs
Copy link
Member

mejrs commented Apr 6, 2022

Can you expand on your use case for this? I wonder what the right api for this will be, because I really don't want to have functions with that many arguments.

@chris-laplante
Copy link
Contributor Author

chris-laplante commented Apr 6, 2022

Can you expand on your use case for this? I wonder what the right api for this will be, because I really don't want to have functions with that many arguments.

I am trying to (indirectly) run code that is accessing sys.argv[0], but since it's not set I get an exception at runtime. I cannot modify the code.

Personally I only care about args. I'd be fine with setting all the rest to null.

@adamreichold
Copy link
Member

I am trying to (indirectly) run code that is accessing sys.argv[0], but since it's not set I get an exception at runtime. I cannot modify the code.

Couldn't you just modify sys.argv directly and put in there whatever you want before running that code?

@chris-laplante
Copy link
Contributor Author

Couldn't you just modify sys.argv directly and put in there whatever you want before running that code?

Good point. I guess I'd just inject sys.argv = ["whatever"] before running?

@chris-laplante
Copy link
Contributor Author

chris-laplante commented Apr 6, 2022

Still, I don't think it'd be a bad idea to implement this at least partially? One thing that is annoying about the workaround above is that it changes line numbers around.

@adamreichold
Copy link
Member

adamreichold commented Apr 7, 2022

One thing that is annoying about the workaround above is that it changes line numbers around.

We are possibly discussing different things. I did not mean to prepend this to the source, but to modify the sys module in a separate invocation of PyEval. This should persist, shouldn't it? At leat the code below does print ["whatever"].

Python::with_gil(|py| {
    let sys = py.eval("__import__('sys')", None, None).unwrap();
    py_run!(py, sys, "sys.argv = ['whatever']");
});

Python::with_gil(|py| {
    py.eval("print(__import__('sys').argv)", None, None).unwrap();
});

I think as this is state of the sys module, it should stay modified as long as that module is loaded?

chris-laplante added a commit to Agilent/bytebraise that referenced this issue May 9, 2022
@chris-laplante
Copy link
Contributor Author

Looks like that works, thanks!

I still think there are use cases for implementing a run_ex, don't you? What you gave above certainly works but feels like a hack, especially when the functionality I need is already present in PyEval_EvalCodeEx.

@adamreichold
Copy link
Member

adamreichold commented May 10, 2022

I still think there are use cases for implementing a run_ex, don't you?

Since the workaround is actually more general (being able to express arbitrary preparations instead of just patching up sys.argv), I would actually prefer it. (It can also certainly be written in a nicer fashion using PyModule::import and PyAny::setattr.)

Therefore, I would prefer to not add to PyO3's API surface without a definite requirement.

@davidhewitt
Copy link
Member

For the sake of throwing ideas into the ring, I'd be happy to hear proposals of what an implementation of a builder pattern would look like for this. I've always found the Python::run and Python::eval APIs not immediately obvious in how they differ (takes reading their docs to remember), and there's only py_run! but no py_eval! macro.

A more flexible pattern with extended and clearer functionality would be welcome. Similarly I'm not keen for run_ex as proposed above.

@chris-laplante
Copy link
Contributor Author

For the sake of throwing ideas into the ring, I'd be happy to hear proposals of what an implementation of a builder pattern would look like for this. I've always found the Python::run and Python::eval APIs not immediately obvious in how they differ (takes reading their docs to remember), and there's only py_run! but no py_eval! macro.

A more flexible pattern with extended and clearer functionality would be welcome. Similarly I'm not keen for run_ex as proposed above.

Sounds good, I'll work on adding a builder. That being said, can you please provide a bit of guidance on the original error message I posted, i.e.:

The error at the args line looks like this:

= note: expected raw pointer `*mut *mut pyo3_ffi::PyObject`
           found raw pointer `*mut pyo3_ffi::PyObject`

It seems like I am passing a pointer to the tuple object, but the method is expecting an array of the tuple elements?

@davidhewitt
Copy link
Member

Looks like it should actually be *const *mut PyObject - see #2368.

The easiest way to get one would be to start with &[&PyAny], call .as_ptr() to get *const &PyAny and then cast that to *const *mut ffi::PyObject. (&PyAny is layout equivalent with *mut ffi::PyObject).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants