Skip to content

python: Add conversions for Duration and Timestamp #263

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

Merged
merged 15 commits into from
Mar 4, 2025

Conversation

gasmith
Copy link
Contributor

@gasmith gasmith commented Feb 28, 2025

This change adds the following conversions into Duration and Timestamp for python:

  • Duration.from_secs(secs: float)
  • Duration.from_timedelta(td: datetime.timedelta)
  • Timestamp.from_epoch_secs(timestamp: float)
  • Timestamp.from_datetime(dt: datetime.datetime)

@gasmith gasmith self-assigned this Feb 28, 2025
Copy link

linear bot commented Feb 28, 2025

Base automatically changed from gasmith/fg-10472-pytime to main February 28, 2025 22:15
@gasmith gasmith force-pushed the gasmith/fg-10472-pytime-conv branch from fa7afda to eafbb8d Compare February 28, 2025 22:15
@gasmith gasmith requested review from eloff and bryfox February 28, 2025 22:19
@gasmith gasmith marked this pull request as ready for review February 28, 2025 22:24
Copy link
Contributor

@bryfox bryfox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth putting this in one of the examples (live-vizualization?) but looks good to me.

@gasmith
Copy link
Contributor Author

gasmith commented Feb 28, 2025

Might be worth putting this in one of the examples (live-vizualization?) but looks good to me.

Oh yeah, I meant to do that, but forgot.

#[pyclass(module = "foxglove.schemas")]
#[derive(Clone)]
#[pyclass(module = "foxglove.schemas", eq)]
#[derive(Clone, PartialEq)]
pub struct Duration(pub(crate) foxglove::schemas::Duration);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do the access specifiers here prevent the user from constructing a Timestamp/Duration out of raw values? If not, I wonder if we may need manual implementations of ==, because the type system doesn't actually prevent nsec from being outside the [0, 1e9) range so there may be multiple representations of the same value. (Unless we are declaring those "invalid"?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only constructor we expose is the new method below. But, it's true that we're not using that opportunity for normalization as we should be.

Copy link
Contributor Author

@gasmith gasmith Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, now normalization is done eagerly when constructing Duration and Timestamp. Here's how things shook out:

  • Rust:
    • Removed public visibility on sec and nsec fields.
    • Added new and new_checked constructors.
    • Added getters for sec and nsec values.
  • Python:
    • Use new_checked constructor, and raise OverflowError when out of range.
    • Add getters for sec and nsec values.

@gasmith gasmith force-pushed the gasmith/fg-10472-pytime-conv branch from f1f2175 to 412f125 Compare March 3, 2025 17:52
from typing import Optional

class Duration:
"""
A duration in seconds and nanoseconds
"""

sec: int
nsec: int

def __new__(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just curious if there is a reason we defined these as __new__ instead of __init__? I suspect since this is a .pyi only there's effectively no difference, other than I feel like __new__ is more commonly customized in python

Copy link
Contributor Author

@gasmith gasmith Mar 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dunno, I'm just following the convention from schemas.pyi, for better or worse.

In python, __new__ is responsible for allocating an instance, and __init__ is responsible for initializing it (docs). When you're working in python, you typically implement __init__, except for unusual circumstances like metaclasses.

Since we're documenting the type interface, I'm not sure the distinction matters all that much.

ChatGPT says it's preferred to document the constructor as __new__ in a .pyi for an extension class, but I'm not very convinced by its line of argument.

I think as long as IDEs and LSPs understand that this is the constructor interface, it probably doesn't matter too much. FWIW, pyright seems to understand it as such.

Comment on lines 74 to 76
let utc = timezone_utc(py);
let epoch = PyDateTime::new(py, 1970, 1, 1, 0, 0, 0, 0, Some(&utc)).unwrap();
let td = dt.call_method1(py, "__sub__", (epoch,))?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we doing this stuff to avoid total_seconds intentionally? I see the docs for that method say:

Note that for very large time intervals (greater than 270 years on most platforms) this method will lose microsecond accuracy.

But they also say

For interval units other than seconds, use the division form directly (e.g. td / timedelta(microseconds=1)).

So I wonder if we could use this division feature to extract the total microseconds directly, rather than having to extract each of the components?

I think what you've done is probably fine, but the manual multiplication by 24*3600 just triggered my "dates are hard"-o-meter a little bit, so I was curious if there's a way to avoid it and make python do the hard work for us.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see it's also documented that "assumes there are exactly 3600*24 seconds in every day" 😅
Might at least be worth some more comments on this stuff

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does look like perhaps / loses full precision while // is able to retain it:

>>> (datetime.fromtimestamp(300*365*24*60*60,UTC)+timedelta(microseconds=1)-datetime.fromtimestamp(0,UTC))/timedelta(microseconds=1)
9460800000000000.0
>>> (datetime.fromtimestamp(300*365*24*60*60,UTC)+timedelta(microseconds=1)-datetime.fromtimestamp(0,UTC))//timedelta(microseconds=1)
9460800000000001

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooo and there's also divmod 😄 this might be my favorite so far since it gives us directly the result we want

>>> divmod((datetime.fromtimestamp(300*365*24*60*60,UTC)+timedelta(microseconds=1)-datetime.fromtimestamp(0,UTC)), timedelta(seconds=1))
(9460800000, datetime.timedelta(microseconds=1))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we doing this stuff to avoid total_seconds intentionally?

Yes. If the caller is trying to convert a datetime, which is already integral, there's no point in bouncing that over to floating point and losing precision along the way.

I see it's also documented that "assumes there are exactly 3600*24 seconds in every day"

Right. That's unix time for you. Every day is 86,400 seconds, no exceptions.

ooo and there's also divmod 😄 this might be my favorite so far since it gives us directly the result we want

It's possible, but it's a lot more fiddly work than what I currently have. We'd need to import divmod from builtins, construct the timedelta(seconds=1) divisor, call divmod, extract the (u32, PyAny) result, downcast the PyAny into a PyDelta, getattr microseconds and extract as u32, and finally multiply by 1000. Not sure I see the benefit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is an argument for putting even more of the interface in pure python, rather than in rust. We could have a pure-python wrapper with various constructors around a more spartan rust class. Seems like a lot of work, though, to accommodate this one particular case.

@gasmith gasmith merged commit 3659088 into main Mar 4, 2025
36 checks passed
@gasmith gasmith deleted the gasmith/fg-10472-pytime-conv branch March 4, 2025 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants