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

Replace humantime crate with jiff #7261

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

oherrala
Copy link

@oherrala oherrala commented Mar 10, 2025

Rationale for this change

The crate humantime appears to be unmaintained. There's open PR in RustSec's advisory-db about this: rustsec/advisory-db#2249

The crates clap and env_logger have already made the switch from humantime to jiff:

Since new version of env_logger is already published the object_store crate is now second largest user of humantime crate by crates.io downloads counts and the largest user after new version of clap is released.

What changes are included in this PR?

Replace humantime crate with jiff. The functionality should stay the same.

Are there any user-facing changes?

There should be none.

Fixes #7264

@github-actions github-actions bot added the object-store Object Store Interface label Mar 10, 2025
@tustvold tustvold added api-change Changes to the arrow API next-major-release the PR has API changes and it waiting on the next major version labels Mar 10, 2025
@tustvold
Copy link
Contributor

tustvold commented Mar 10, 2025

Thank you, as jiff isn't 100% compatible with humantime I think this constitutes a breaking change and will therefore have to wait for the next breaking release in likely ~6 months time...

That being said now that an advisory has been published we might want to just push ahead with this to avoid undue angst... I'll let other maintainers weigh in

Edit: tbh given how small humantime actually is we probably could just vend the parser locally as a temporary measure... AFAICT there isn't actually an issue with the code

@BurntSushi
Copy link

AFAICT there isn't actually an issue with the code

It depends on what you're doing, but humantime will happily treat 1 month as 30.44 days, regardless of which month it is. Similarly for other variable length units, like years and days. The discussion in the Jiff docs shows examples, and specifically how Jiff avoids this pitfall.

With that said, in this PR, the impl Parse for Duration would previously accept 1month (translating it to 30.44 24-hour days), but the implementation in this PR will reject anything with non-zero units greater than hours. So 1 day would be rejected here, for example. In order to parse calendar units, you need to use Span. And to resolve that to a real duration in time, you need a relative datetime. (Which is more annoying and presumably why humantime decided to implicitly convert calendar units to their "average" value, despite it producing unintuitive results.)

@tustvold
Copy link
Contributor

Given these are timeouts I suspect few users are configuring anything larger than an hour. Are there any differences when parsing such?

grtlr added a commit to rerun-io/rerun that referenced this pull request Mar 11, 2025
Fixes CI failures due to `humantime` being marked as unmaintained.

It looks like it will take some time for `object_store` to move on, so
I'm adding
[`RUSTSEC-2025-0014`](https://rustsec.org/advisories/RUSTSEC-2025-0014)
to the ignore list for now:

* apache/arrow-rs#7264
* apache/arrow-rs#7261
* Reminder issue: #9255

---------

Co-authored-by: Andreas Reich <andreas@rerun.io>
@BurntSushi
Copy link

BurntSushi commented Mar 11, 2025

Given these are timeouts I suspect few users are configuring anything larger than an hour. Are there any differences when parsing such?

That's a good point and a good question. The major difference is definitely that parsing into SignedDuration will reject any duration with non-zero year/month/week/day units. But aside from that, I think it depends on which direction of compatibility you're interested in. If it's just a matter of what humantime supports in terms of parsing, we can take a look at its implementation:

        let (mut sec, nsec) = match &self.src[start..end] {
            "nanos" | "nsec" | "ns" => (0u64, n),
            "usec" | "us" => (0u64, n.mul(1000)?),
            "millis" | "msec" | "ms" => (0u64, n.mul(1_000_000)?),
            "seconds" | "second" | "secs" | "sec" | "s" => (n, 0),
            "minutes" | "minute" | "min" | "mins" | "m"
            => (n.mul(60)?, 0),
            "hours" | "hour" | "hr" | "hrs" | "h" => (n.mul(3600)?, 0),
            "days" | "day" | "d" => (n.mul(86400)?, 0),
            "weeks" | "week" | "w" => (n.mul(86400*7)?, 0),
            "months" | "month" | "M" => (n.mul(2_630_016)?, 0), // 30.44d
            "years" | "year" | "y" => (n.mul(31_557_600)?, 0), // 365.25d
            _ => {
                return Err(Error::UnknownUnit {
                    start, end,
                    unit: self.src[start..end].to_string(),
                    value: n,
                });
            }
        };

Jiff's "friendly" duration format has a grammar, and indeed, for hour/minute/second/millisecond/microsecond/nanosecond units, Jiff can parse all of the units that humantime can parse (plus more, including things like µs). In general, in terms of things that humantime can parse but Jiff can't, I believe the only thing is M for months. Jiff requires at least mo or mos.

The other direction is different, that is, can humantime parse what Jiff outputs. I'm not sure if you need that form of compatibility, but if you do, Jiff has a special HumanTime mode for it.

jprochazk pushed a commit to rerun-io/rerun that referenced this pull request Mar 11, 2025
Fixes CI failures due to `humantime` being marked as unmaintained.

It looks like it will take some time for `object_store` to move on, so
I'm adding
[`RUSTSEC-2025-0014`](https://rustsec.org/advisories/RUSTSEC-2025-0014)
to the ignore list for now:

* apache/arrow-rs#7264
* apache/arrow-rs#7261
* Reminder issue: #9255

---------

Co-authored-by: Andreas Reich <andreas@rerun.io>
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

That being said now that an advisory has been published we might want to just push ahead with this to avoid undue angst... I'll let other maintainers weigh in

I think this is a good change and that the removal of a RUSTSEC is worth any potential issue with parsing questionable durations.

Given @BurntSushi 's long history of maintaining core crates in the Rust ecosystem and the current adoption of jiff I think this is a good choice

https://crates.io/crates/jiff

BTW if/when this PR is merged, I think we can/should consider backporting this change to the object_store 0.11 line so others can pick up the change before 0.12.0 percolates through the ecosystem

westonpace added a commit to lancedb/lance that referenced this pull request Mar 12, 2025
The `humantime` package is no longer maintained. We should move to avoid
using it. We were using it indirectly via `env_logger` and
`object_store`. This PR updates `env_logger` to the latest version
(which uses `jiff` instead of `humantime`).

`object_store` is one of our core dependencies and we generally update
it as new versions are released. If (and when) `humantime` is replaced
with `jiff` we will remove the dependency then. This may be several
months. apache/arrow-rs#7261 can be used for
tracking.

Until then we should suppress the advisory.
@yhx-12243
Copy link

humantime seems resurrected now (1 hour ago).

@alamb
Copy link
Contributor

alamb commented Mar 13, 2025

humantime seems resurrected now (1 hour ago).

🤔

it does appear they are starting to make code and releases again:
https://github.com/chronotope/humantime/commits/main/

https://crates.io/crates/humantime/2.2.0

@alamb
Copy link
Contributor

alamb commented Mar 13, 2025

Indeed, it seems like @djc has stepped up to be a maintainer. 🙏

@Xuanwo
Copy link
Member

Xuanwo commented Mar 13, 2025

Oh, @djc saved another crate!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change Changes to the arrow API next-major-release the PR has API changes and it waiting on the next major version object-store Object Store Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[object_store] consider migrating humantime to jiff
6 participants