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

Unable to use PyTzInfo #1662

Closed
samuelcolvin opened this issue Jun 5, 2021 · 9 comments
Closed

Unable to use PyTzInfo #1662

samuelcolvin opened this issue Jun 5, 2021 · 9 comments

Comments

@samuelcolvin
Copy link
Contributor

samuelcolvin commented Jun 5, 2021

Question / Clarification

🌍 Environment

  • Your operating system and version: Ubuntu 21.04
  • Your python version:3.9.5
  • How did you install python (e.g. apt or pyenv)? Did you use a virtualenv?: system
  • Your Rust version (rustc --version): rustc 1.52.1 (9bc8c42bb 2021-05-09)
  • Your PyO3 version: 0.13.2
  • Have you tried using latest PyO3 main (replace version = "0.x.y" with git = "https://github.com/PyO3/pyo3")?: no

Details

As described at #884 (comment) and in tests here, I assumed should be able to "subclass" PyTzInfo and use it in PyDateTime::new, but I'm getting

error[E0308]: mismatched types
  --> src/lib.rs:84:22
   |
84 |                 Some(&tz_info)
   |                      ^^^^^^^^ expected struct `Py`, found struct `TzClass`
   |
   = note: expected reference `&Py<PyAny>`
              found reference `&TzClass`

What am I doing wrong?

partial code:

fn chrono_py_err(e: ParseError) -> PyErr {
    PyErr::new::<PyValueError, _>(PyValueError::new_err(e.to_string()))
}

#[pyclass(extends=PyTzInfo)]
struct TzClass {
    seconds: i32
}

#[pymethods]
impl TzClass {
    #[new]
    fn new(seconds: i32) -> Self {
        TzClass {seconds}
    }

    fn utcoffset<'p>(&self, py: Python<'p>, _dt: &PyDateTime) -> PyResult<&'p PyDelta> {
        PyDelta::new(py, 0, self.seconds, 0, true)
    }

    fn tzname(&self, _py: Python<'_>, _dt: &PyDateTime) -> String {
        String::from("+01:00")  // TODO
    }

    fn dst(&self, _py: Python<'_>, _dt: &PyDateTime) -> Option<&PyDelta> {
        None
    }
}

...
            let offset_seconds = dt.offset().local_minus_utc();
            let tz_info = TzClass::new(offset_seconds);
            let dt = PyDateTime::new(
                py,
                ...
                Some(&tz_info)
            )?;

Full example of my (very naive) attempt at solving this: samuelcolvin/rtoml#25

@davidhewitt
Copy link
Member

Indeed; this is a similar problem to the changes being looked at in #1588.

In particular, there's two related pain points:

  • PyDateTime::new takes Option<&Py<PyAny>> rather than Option<&PyTzInfo>.
  • &PyCell<TzClass> implements Deref<Target = PyAny>; I think it makes sense for it to implement Deref<Target = PyTzInfo> (i.e. #[pyclass] should deref to the super type rather than directly to PyAny)

For now you can work around this by using Some(&tz_info.into_py(py)) instead of Some(&tz_info). Let's keep this issue open as I'm very tempted to improve the situation for either 0.14 or 0.15.

@samuelcolvin
Copy link
Contributor Author

Humm, I've tried that but I'm getting

error[E0599]: the method `into_py` exists for struct `TzClass`, but its trait bounds were not satisfied
  --> src/lib.rs:83:31
   |
22 | struct TzClass {
   | --------------
   | |
   | method `into_py` not found for this
   | doesn't satisfy `TzClass: AsPyPointer`
...
83 |                 Some(&tz_info.into_py(py)),
   |                               ^^^^^^^ method cannot be called on `TzClass` due to unsatisfied trait bounds
   |
   = note: the following trait bounds were not satisfied:
           `TzClass: AsPyPointer`
           which is required by `&TzClass: pyo3::IntoPy<Py<PyAny>>`

@samuelcolvin
Copy link
Contributor Author

I've updated the above PR with your suggestion in case seeing the error in CI helps, but I have no idea how to go about fixing this.

@davidhewitt
Copy link
Member

Unngh, sorry. How about Some(&Py::new(py, tz_info)?.into())?

The APIs on Rust <-> Python interface boundary need some love when even I'm getting them wrong 🤦‍♂️

@samuelcolvin
Copy link
Contributor Author

Nope, I'm getting

error[E0277]: the trait bound `TzClass: AsRef<PyAny>` is not satisfied
  --> src/lib.rs:96:45
   |
96 |                 Some(&Py::new(py, tz_info)?.into()),
   |                                             ^^^^ the trait `AsRef<PyAny>` is not implemented for `TzClass`
   |
   = note: required because of the requirements on the impl of `From<pyo3::Py<TzClass>>` for `pyo3::Py<PyAny>`
   = note: required because of the requirements on the impl of `Into<pyo3::Py<PyAny>>` for `pyo3::Py<TzClass>`

error: aborting due to previous error

@samuelcolvin
Copy link
Contributor Author

What is working if it helps, is the following extremely ugly workaround:

let locals = PyDict::new(py);
locals.set_item("datetime", py.import("datetime")?)?;
locals.set_item("seconds", offset_seconds.to_object(py))?;
let code = "datetime.timezone(datetime.timedelta(seconds=seconds))";
let tz_info = py.eval(code, None, Some(&locals))?;

Then Some(&tz_info.into_py(py)) as you suggested before.

But obviously a solution without dropping back to python would be good (not yet sure how slow the above is).

@davidhewitt
Copy link
Member

Ok, last try: Some(&Py::new(py, tz_info)?.to_object(py))

... I think that at least one of the other methods not working is a design flaw. Sorry that you've had to suffer this.

@samuelcolvin
Copy link
Contributor Author

Okay, that works, thanks so much.

(at least it wasn't me missing something obvious)

@davidhewitt
Copy link
Member

#1588 changed Datetime::new to take Option<&PyTzInfo>, so I'm going to close this issue as resolved.

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

2 participants