[cron] replace croniter with something that doesn't suck#33952
[cron] replace croniter with something that doesn't suck#33952sbquinlan wants to merge 3 commits into
Conversation
Greptile SummaryThis PR replaces Dagster's vendored
Confidence Score: 5/5Safe to merge; public scheduling APIs are unchanged and the PR ships a compatibility test suite alongside the new native implementation. The change is well-scoped, replacing an internal implementation while keeping public APIs identical. The croniter compatibility suite, Dagster integration tests, and DST-handling logic in Rust all provide good coverage. The only findings are minor: a dead guard that can never fire and an unpinned CI dependency. python_modules/libraries/dagster-cron/Cargo.toml — the git-branch dependency on a personal fork should be replaced with a versioned crates.io release once the upstream cron crate merges the required features.
|
| Filename | Overview |
|---|---|
| python_modules/libraries/dagster-cron/src/lib.rs | New Rust/PyO3 core: exposes Schedule, ScheduleIterator, CronStringIterator, and top-level helper functions. Contains one dead guard in new_dagster_schedule (always-false condition) but the real validation occurs in normalize_dagster_expression, so runtime behaviour is unaffected. |
| python_modules/dagster/dagster/_utils/schedules.py | Major simplification: removes ~600 lines of Python cron iteration logic and delegates to dagster_cron native functions. Adds batched_cron_string_iterator and cron_string_iterator_batch helpers, and keeps has_out_of_range_cron_interval as pure Python field validation. |
| python_modules/libraries/dagster-cron/Cargo.toml | Declares the maturin/PyO3 package with a git-branch dependency on the author's personal fork of the cron crate; Cargo.lock pins a specific commit so --locked builds are reproducible, but the branch reference is a temporary supply-chain risk. |
| .github/workflows/build-dagster-cron-wheels.yml | New wheel-build workflow for Linux, macOS-14 (arm64), and Windows. Unpinned maturin install could cause non-reproducible builds; all other toolchain steps use version-tagged actions. |
| python_modules/dagster/dagster/_core/definitions/auto_materialize_rule_impls.py | Switches missed_cron_ticks from unbatched cron_string_iterator to batched_cron_string_iterator; semantics are identical (early-return on first tick past evaluation_time). |
| python_modules/dagster/dagster/_core/definitions/partitions/definition/time_window.py | Replaces next(cron_string_iterator(...)) == window_start with cron_string_includes(window_start.timestamp(), ...) for exclusion checks — semantically equivalent and more efficient. |
| python_modules/libraries/dagster-cron/python/dagster_cron/init.pyi | Comprehensive stub file covering all exported types and functions with correct signatures. |
Reviews (2): Last reviewed commit: "Add native cron batch helpers" | Re-trigger Greptile
| [dependencies] | ||
| chrono = { version = "~0.4", default-features = false, features = ["clock"] } | ||
| chrono-tz = "0.10" | ||
| cron = { git = "https://github.com/sbquinlan/cron.git", branch = "codex/ci-supply-chain-hygiene" } |
There was a problem hiding this comment.
Mutable git branch dependency on a personal fork
cron is pinned to a branch (codex/ci-supply-chain-hygiene) on the author's personal fork rather than a released crates.io version. The PR description acknowledges this is temporary while upstream PRs land. The Cargo.lock pins commit d74e27a5437baab1dee83d9a9238bcd4dcdd9472, so --locked builds are reproducible today, but cargo update would silently follow the branch tip. A force-push, branch deletion, or a supply-chain compromise of the fork would affect any rebuild that does not use the lock file. Once the upstream cron crate merges and releases the required features, this should be replaced with a versioned crates.io dependency.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
@gibsondan can you unblock CI for me please? |
Note: currently the PRs to add croniter compatibility to the cron crate are still landing. This pr adds a dependency to a specific branch
cron = { git = "https://github.com/sbquinlan/cron.git", branch = "codex/ci-supply-chain-hygiene" }. I'm open to vendoring the entirety of the cron crate here or any other alternative. Unfortunately review is slow in the cron crate.Summary & Motivation
Replace Dagster's vendored
croniterusage and custom Python cron iteration fast paths with a small Rust-backed package,dagster-cron. dagster-cron offers 20x to 100x performance improvements over croniter's implementation.Dagster's public scheduling API remains unchanged: callers still use
is_valid_cron_string,cron_string_iterator,reverse_cron_string_iterator,schedule_execution_time_iterator, and the existing time partition APIs. Internally, those functions now delegate todagster_cronas quickly as possible instead of routing through croniter or Dagster's custom Python_find_*iteration helpers.Changes
python_modules/libraries/dagster-cron, amaturin/PyO3 package exposing a thin native surface:Schedule,ScheduleIterator,CronStringIterator,includes, forward/reverse iteration, and the small set of schedule configuration enums Dagster needs.dagster._utils.schedulescron iteration with calls intodagster_cron.dagster._vendored.has_out_of_range_cron_intervalas raw Python field validation because it does not need cron iteration.dagster-cronacross Linux, macOS, and Windows.dagster-cronto Dagster dependency/dev-install/lockfile wiring.Compatibility
No breaking changes are intended for Dagster's public scheduling APIs or time partition APIs.
Supported through the Dagster runtime path:
@hourly,@daily,@weekly,@monthly,@yearly.@annuallyand@midnight, normalized in Rust.0or7.?in day-of-month and day-of-week fields.L, nearest weekdayW, and nth weekday#.Rfields.Intentional or known unsupported croniter surfaces:
Hexpressions. (not relevant in dagster)@reboot. (also not relevant)expand_from_start_time. (flag not used by dagster)implement_cron_bug. (flag not used by dagster)Validation notes:
Test Plan
cargo test --lockedcargo fmt --checkuv lock --checkinpython_modules/libraries/dagster-cronuv lock --checkinpython_modules/dagsteruv run --group test --reinstall-package dagster-cron pytest -quv run --group test python tests/test_croniter_compatibility.py --skip-known-unsupported -q140 passed, 75 skippedChangelog
Dagster cron schedule validation and iteration now use the Rust-backed
dagster-cronpackage instead of vendoredcroniterand Dagster's previous custom Python cron iteration implementation. Public scheduling APIs are unchanged.