test(pyaugurs): add comprehensive test suite with 120 tests#402
Conversation
|
Warning Rate limit exceeded@sd2k has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
WalkthroughThe PR renames the DBSCAN constructor parameter from Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Model as ETS/MSTL
Caller->>Model: predict(horizon, level...)
rect rgba(220,230,255,0.5)
Model->>Model: if horizon == 0
Note right of Model: new precondition check
end
alt horizon == 0
Model-->>Caller: PyException("horizon must be greater than 0")
else horizon > 0
Model->>Model: delegate to inner.predict(horizon)
Model-->>Caller: Forecast (point, lower, upper)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/pyaugurs/src/clustering.rs (1)
46-50: All Python test calls must be updated fromeps=toepsilon=—this is a breaking change that breaks 27+ test invocations.The Rust constructor at line 65 now uses
epsilon: f64as the parameter name, and the__repr__at line 60 displays"Dbscan(epsilon=...)". However, the test filecrates/pyaugurs/tests/test_clustering.pycontains 27+ calls likeDbscan(eps=0.5, min_cluster_size=2)that will fail because PyO3 maps Rust parameter names directly to Python keyword arguments.Either update all tests to
Dbscan(epsilon=..., min_cluster_size=...)or add a backwards-compatible Python wrapper that accepts bothepsandepsilon.
🧹 Nitpick comments (8)
crates/pyaugurs/tests/test_seasons.py (4)
51-75: Consider seeding random number generation for reproducible tests.Multiple fixtures/tests use
np.random.randnwithout a fixed seed, which makes results non‑reproducible across runs and can make future, stricter assertions harder to debug if behavior shifts. Consider using a module‑level RNG with a fixed seed (e.g.,rng = np.random.default_rng(123)andrng.normal(...)) or a pytest fixture that provides a seeded RNG.Also applies to: 172-177, 190-195, 225-231, 279-284
213-223: Cast periods to Python ints before subtracting to avoid unsigned wraparound.
periodsis an unsigned NumPy dtype (validated intest_returns_uint_array), sop - 8underflows forp < 8, andabs(p - 8)becomes a huge positive value. Casting tointbefore subtraction makes the “within ±1 of 8” check robust.- # Should definitely detect the period of 8 - assert any(abs(p - 8) <= 1 for p in periods) + # Should definitely detect the period of 8 (allow small tolerance) + assert any(abs(int(p) - 8) <= 1 for p in periods)
160-165: Align “no seasonality” docstrings with assertions (or strengthen checks).
test_no_seasonality,test_with_constant_series, andtest_with_linear_trend_onlyall describe data that should have no (or no significant) seasonality, but only assert thatseasonalitiesreturns a NumPy array. Either add expectations about the result (e.g.,len(periods) == 0or an upper bound on the number of detected periods) or relax the docstrings to clarify that these are smoke tests checking that the function handles such inputs without error.Also applies to: 199-212, 225-234
51-60: (Optional) Use deterministic RNG fixtures for noisy seasonal patterns.For the noisy seasonal fixtures (
seasonal_data_period_7,seasonal_data_period_12,multi_seasonal_data,non_seasonal_data), consider centralizing noise generation through a seeded RNG fixture so that failures are reproducible and data properties stay stable over time, even if global NumPy RNG behavior changes.Also applies to: 63-69, 72-74
crates/pyaugurs/src/ets.rs (1)
69-79: Horizon validation is good; consider updating the error docsThe
horizon == 0guard with aPyException("horizon must be greater than 0")matches the new tests and prevents confusing zero-length forecasts. It would be helpful to mention this additional failure mode in the Rustdoc “Errors” section forpredict.crates/pyaugurs/tests/test_ets.py (1)
201-218: Consider tighteningpytest.raises(Exception)and clarifying what you’re testingFor invalid horizons and “predict before fit” you currently assert
pytest.raises(Exception). This passes, but:
- For
horizon=0you’re exercising the explicitPyException("horizon must be greater than 0")check.- For
horizon=-1the failure happens earlier during Python→usizeconversion, so you’re not actually exercising the Rust guard.If you want stricter tests and to satisfy Ruff’s B017, consider asserting the specific exception types/messages you expect for each case (and possibly dropping the
-1case if you don’t care about the conversion error semantics).crates/pyaugurs/src/mstl.rs (1)
89-98: MSTL horizon guard mirrors ETS and is appropriateThe early return on
horizon == 0withPyException("horizon must be greater than 0")is consistent withAutoETS.predictand with the new MSTL tests, and avoids zero-length forecasts. It’d be useful to update the Rustdoc forpredictto mention this additional error condition alongside thelevelconstraints.crates/pyaugurs/tests/test_mstl.py (1)
238-262: Optionally narrowpytest.raises(Exception)and reconsider the-1horizon caseFor the “predict before fit” and “invalid horizon” tests you’re using
pytest.raises(Exception), which triggers Ruff’s B017 and makes the assertions less specific. Also, forhorizon=-1the failure will come from Python→usizeconversion, not from the Rusthorizon == 0guard.If you care about lint cleanliness and stronger guarantees, consider:
- Asserting the concrete exception types/messages you actually see for each case, and/or
- Dropping the
-1case if your intent is just to validate thehorizon > 0precondition.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
crates/pyaugurs/src/clustering.rs(2 hunks)crates/pyaugurs/src/ets.rs(2 hunks)crates/pyaugurs/src/mstl.rs(1 hunks)crates/pyaugurs/tests/test_clustering.py(1 hunks)crates/pyaugurs/tests/test_dtw.py(3 hunks)crates/pyaugurs/tests/test_ets.py(1 hunks)crates/pyaugurs/tests/test_mstl.py(1 hunks)crates/pyaugurs/tests/test_seasons.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-12T13:54:18.009Z
Learnt from: sd2k
Repo: grafana/augurs PR: 0
File: :0-0
Timestamp: 2025-01-12T13:54:18.009Z
Learning: In the augurs clustering implementation, the `DbscanCluster` enum with `Cluster(NonZeroU32)` and `Noise` variants is preferred over using `isize` with `-1` for noise points, as it provides better type safety and self-documentation despite being more verbose.
Applied to files:
crates/pyaugurs/src/clustering.rs
🧬 Code graph analysis (6)
crates/pyaugurs/tests/test_seasons.py (2)
crates/pyaugurs/src/lib.rs (1)
augurs(106-116)crates/augurs-seasons/src/periodogram.rs (3)
min_period(35-38)max_period(44-47)threshold(55-58)
crates/pyaugurs/src/clustering.rs (1)
crates/augurs-clustering/src/lib.rs (3)
epsilon(86-88)min_cluster_size(92-94)new(77-82)
crates/pyaugurs/tests/test_ets.py (2)
crates/pyaugurs/src/lib.rs (1)
augurs(106-116)crates/pyaugurs/src/ets.rs (3)
fit(51-58)predict(69-79)predict_in_sample(90-97)
crates/pyaugurs/tests/test_mstl.py (2)
crates/pyaugurs/src/lib.rs (1)
augurs(106-116)crates/pyaugurs/src/mstl.rs (4)
ets(37-45)fit(77-83)predict(90-98)predict_in_sample(105-110)
crates/pyaugurs/tests/test_clustering.py (4)
crates/pyaugurs/src/lib.rs (1)
augurs(106-116)demo/clustering.worker.js (3)
clusterer(12-12)labels(13-13)dtw(10-10)crates/augurs-clustering/src/lib.rs (1)
min_cluster_size(92-94)crates/pyaugurs/src/clustering.rs (1)
fit(81-95)
crates/pyaugurs/tests/test_dtw.py (1)
crates/pyaugurs/src/distance.rs (1)
to_numpy(46-54)
🪛 GitHub Actions: Python tests
crates/pyaugurs/tests/test_clustering.py
[error] 1-1: Dbscan.new() got an unexpected keyword argument 'eps' during instantiation in multiple tests. The Dbscan constructor likely does not accept 'eps'; this causes 25 test failures. Update constructor signature or test usage accordingly.
🪛 Ruff (0.14.6)
crates/pyaugurs/tests/test_ets.py
206-206: Do not assert blind exception: Exception
(B017)
209-209: Do not assert blind exception: Exception
(B017)
216-216: Do not assert blind exception: Exception
(B017)
crates/pyaugurs/tests/test_mstl.py
242-242: Do not assert blind exception: Exception
(B017)
249-249: Do not assert blind exception: Exception
(B017)
257-257: Do not assert blind exception: Exception
(B017)
260-260: Do not assert blind exception: Exception
(B017)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Clippy
- GitHub Check: Test Book
- GitHub Check: Run benchmarks
- GitHub Check: Check
- GitHub Check: Tests
- GitHub Check: linux (ubuntu-22.04, armv7)
- GitHub Check: windows (windows-2022, x86)
- GitHub Check: musllinux (ubuntu-22.04, x86)
- GitHub Check: linux (ubuntu-22.04, aarch64)
- GitHub Check: windows (windows-2022, x64)
- GitHub Check: linux (ubuntu-22.04, ppc64le)
- GitHub Check: linux (ubuntu-22.04, x86_64)
- GitHub Check: linux (ubuntu-22.04, s390x)
- GitHub Check: linux (ubuntu-22.04, x86)
- GitHub Check: macos (macos-13, x86_64)
- GitHub Check: musllinux (ubuntu-22.04, aarch64)
- GitHub Check: musllinux (ubuntu-22.04, armv7)
- GitHub Check: musllinux (ubuntu-22.04, x86_64)
- GitHub Check: Prophet WASMStan component
- GitHub Check: JS tests
🔇 Additional comments (6)
crates/pyaugurs/tests/test_seasons.py (1)
7-287: Extensive seasonality test coverage looks solid overall.The fixtures span a good variety of seasonal and non‑seasonal patterns, and the tests exercise core
seasonalitiesoptions (min_period,max_period,threshold) across different input representations. Aside from the smaller issues noted in other comments, this suite should be a strong guardrail against regressions in the seasons logic.crates/pyaugurs/src/ets.rs (1)
51-57: Persisting the fitted model inself.fittedlooks correctCapturing
inner.fit(...)into a local and then assigningself.fitted = Some(fitted)cleanly enables subsequentpredict/predict_in_samplecalls without changing the public API. Error handling remains unchanged.crates/pyaugurs/tests/test_ets.py (1)
41-200: Comprehensive ETS test coverage looks solidThese tests exercise construction, multiple specs, list/ndarray inputs, horizons, intervals, in-sample forecasts, seasonality, and multiple fits. They align with the new
self.fittedbehaviour and horizon validation inAutoETS.predict, and should give good confidence in the binding.crates/pyaugurs/tests/test_mstl.py (1)
49-237: MSTL test suite is thorough and aligned with the implementationThese tests cover ETS-backed instantiation, multiple seasonal periods, various input types, horizons, confidence levels, in-sample vs out-of-sample forecasts, multiple fits, and realistic multi-seasonal patterns. They line up well with
MSTL.ets,fit,predict, andpredict_in_samplesemantics and should give strong confidence in the binding.crates/pyaugurs/tests/test_dtw.py (2)
7-35: DTW instantiation tests nicely cover the configuration surfaceThe new
TestDtwInstantiationcases ensureDtwcan be constructed with defaults, explicit distance functions, window-only, and combined parameters, which is a good sanity check on the public constructor contract.
43-47: Extended DTW distance and distance_matrix tests look correct and robustThe added cases around explicit
distance_fn, typed arrays, differing lengths, empty inputs, Manhattan distance, symmetry, single/large distance matrices, reuse of aDtwinstance, and realistic sinusoidal series collectively give strong coverage of bothdistanceanddistance_matrix. The numeric expectations appear consistent with the existing DTW semantics and should help catch regressions without being overly brittle.Also applies to: 66-76, 84-314
| def test_seasonality_with_float64_array(self, seasonal_data_period_4): | ||
| """Test seasonality detection with explicit float64 array.""" | ||
| periods = seasonalities(np.float64(seasonal_data_period_4)) | ||
| assert isinstance(periods, np.ndarray) | ||
| assert len(periods) > 0 | ||
| assert 4 in periods |
There was a problem hiding this comment.
Fix np.float64 usage that will raise on non‑scalar arrays.
np.float64(seasonal_data_period_4) attempts to coerce the entire array to a single scalar and will raise TypeError for length > 1, so this test will fail before calling seasonalities. Use an array cast instead.
- def test_seasonality_with_float64_array(self, seasonal_data_period_4):
- """Test seasonality detection with explicit float64 array."""
- periods = seasonalities(np.float64(seasonal_data_period_4))
+ def test_seasonality_with_float64_array(self, seasonal_data_period_4):
+ """Test seasonality detection with explicit float64 array."""
+ periods = seasonalities(seasonal_data_period_4.astype(np.float64))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_seasonality_with_float64_array(self, seasonal_data_period_4): | |
| """Test seasonality detection with explicit float64 array.""" | |
| periods = seasonalities(np.float64(seasonal_data_period_4)) | |
| assert isinstance(periods, np.ndarray) | |
| assert len(periods) > 0 | |
| assert 4 in periods | |
| def test_seasonality_with_float64_array(self, seasonal_data_period_4): | |
| """Test seasonality detection with explicit float64 array.""" | |
| periods = seasonalities(seasonal_data_period_4.astype(np.float64)) | |
| assert isinstance(periods, np.ndarray) | |
| assert len(periods) > 0 | |
| assert 4 in periods |
🤖 Prompt for AI Agents
In crates/pyaugurs/tests/test_seasons.py around lines 91 to 96, the test calls
np.float64(seasonal_data_period_4) which attempts to coerce the whole array to a
scalar and will raise TypeError for non‑scalar inputs; change the cast to
convert the array to a float64 array instead (for example use
seasonal_data_period_4.astype(np.float64) or np.array(seasonal_data_period_4,
dtype=np.float64)) so the test passes and seasonalities receives an array of
dtype float64.
| def test_multiple_seasonalities(self, multi_seasonal_data): | ||
| """Test detection of multiple seasonal periods.""" | ||
| periods = seasonalities(multi_seasonal_data) | ||
| assert isinstance(periods, np.ndarray) | ||
| # Should detect multiple periods | ||
| # This is data-dependent, so we just check it returns something reasonable | ||
| assert len(periods) >= 0 | ||
|
|
There was a problem hiding this comment.
len(periods) >= 0 is tautological and doesn’t validate behavior.
In test_multiple_seasonalities, len(periods) >= 0 is always true, so the test doesn’t actually check the result. At minimum, assert that at least one period is detected; if you’re confident in the data, you could tighten this to >= 2 to better match the docstring.
- # Should detect multiple periods
- # This is data-dependent, so we just check it returns something reasonable
- assert len(periods) >= 0
+ # Should detect at least one period; loosen if this proves flaky
+ assert len(periods) > 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_multiple_seasonalities(self, multi_seasonal_data): | |
| """Test detection of multiple seasonal periods.""" | |
| periods = seasonalities(multi_seasonal_data) | |
| assert isinstance(periods, np.ndarray) | |
| # Should detect multiple periods | |
| # This is data-dependent, so we just check it returns something reasonable | |
| assert len(periods) >= 0 | |
| def test_multiple_seasonalities(self, multi_seasonal_data): | |
| """Test detection of multiple seasonal periods.""" | |
| periods = seasonalities(multi_seasonal_data) | |
| assert isinstance(periods, np.ndarray) | |
| # Should detect at least one period; loosen if this proves flaky | |
| assert len(periods) > 0 |
🤖 Prompt for AI Agents
In crates/pyaugurs/tests/test_seasons.py around lines 152 to 159, the assertion
`len(periods) >= 0` is meaningless; replace it with a real check that at least
one seasonality is detected (e.g., assert len(periods) >= 1), or if the fixture
is guaranteed to contain multiple seasonalities, tighten to assert len(periods)
>= 2; keep the isinstance check and adjust the assertion message if you add one.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/pyaugurs/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
crates/pyaugurs/tests/test_clustering.py(1 hunks)justfile(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/pyaugurs/tests/test_clustering.py (4)
crates/pyaugurs/src/lib.rs (1)
augurs(106-116)demo/clustering.worker.js (3)
clusterer(12-12)labels(13-13)dtw(10-10)crates/augurs-clustering/src/lib.rs (2)
epsilon(86-88)min_cluster_size(92-94)crates/pyaugurs/src/clustering.rs (1)
fit(81-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: JS tests
- GitHub Check: windows (windows-2022, x86)
- GitHub Check: windows (windows-2022, x64)
- GitHub Check: macos (macos-14, aarch64)
- GitHub Check: macos (macos-13, x86_64)
- GitHub Check: linux (ubuntu-22.04, ppc64le)
- GitHub Check: musllinux (ubuntu-22.04, x86)
- GitHub Check: linux (ubuntu-22.04, s390x)
- GitHub Check: linux (ubuntu-22.04, x86_64)
- GitHub Check: musllinux (ubuntu-22.04, armv7)
- GitHub Check: musllinux (ubuntu-22.04, aarch64)
- GitHub Check: musllinux (ubuntu-22.04, x86_64)
- GitHub Check: linux (ubuntu-22.04, aarch64)
- GitHub Check: linux (ubuntu-22.04, x86)
- GitHub Check: linux (ubuntu-22.04, armv7)
- GitHub Check: Clippy
- GitHub Check: Check
- GitHub Check: Run benchmarks
- GitHub Check: Tests
- GitHub Check: Test Book
- GitHub Check: Prophet WASMStan component
🔇 Additional comments (10)
crates/pyaugurs/tests/test_clustering.py (10)
42-66: LGTM! Past review issue resolved.All Dbscan instantiations correctly use the
epsilonparameter (not the oldepskeyword). The instantiation and repr tests provide good coverage of basic functionality.
68-85: LGTM!Good coverage of both list and numpy array inputs with appropriate type and shape assertions.
86-111: LGTM!Cluster assignment logic is correct based on the distance matrix structure with epsilon=1.0.
112-130: LGTM!Good integration test demonstrating DTW distance computation followed by DBSCAN clustering.
131-160: LGTM!Parameter variation tests provide good smoke test coverage, ensuring different configurations don't crash.
161-222: LGTM!Excellent edge case coverage with correct expected outcomes for all-noise, single-cluster, and multi-cluster scenarios.
223-284: LGTM!Good dtype compatibility tests. Note that the
test_symmetric_distance_matrixandtest_zero_diagonaltests verify that well-formed matrices work but don't validate enforcement of symmetry or zero-diagonal requirements (if the implementation has such validations).
285-341: LGTM!Edge case tests for various matrix sizes provide good boundary condition coverage.
342-384: LGTM!Good validation of README example pattern and alternative distance function (Manhattan).
385-406: LGTM!Excellent test verifying that the clusterer is stateless and can be safely reused across multiple fit operations.
| # Build the Python package | ||
| build-pyaugurs: | ||
| cd crates/pyaugurs && uv pip install -e . --force-reinstall --no-deps | ||
|
|
||
| # Run Python tests | ||
| test-pyaugurs: build-pyaugurs | ||
| cd crates/pyaugurs && uv run pytest tests/ -v |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find dependency configuration files in crates/pyaugurs
fd -t f '(pyproject\.toml|setup\.py|setup\.cfg|requirements.*\.txt)' crates/pyaugursRepository: grafana/augurs
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Read the pyproject.toml file
cat crates/pyaugurs/pyproject.tomlRepository: grafana/augurs
Length of output: 521
🏁 Script executed:
#!/bin/bash
# Check uv documentation and behavior with --no-deps and dependency-groups
# First, let's see if there are any comments in the justfile about this
cat -n justfile | sed -n '50,70p'Repository: grafana/augurs
Length of output: 690
🏁 Script executed:
#!/bin/bash
# Look for uv.lock or other lock files
fd -t f 'uv\.lock|.*\.lock' crates/pyaugurs
# Also check the root directory
fd -t f 'uv\.lock' . | head -5Repository: grafana/augurs
Length of output: 107
🏁 Script executed:
#!/bin/bash
# Check if pytest and numpy are in uv.lock
grep -E '(pytest|numpy)' crates/pyaugurs/uv.lock | head -20Repository: grafana/augurs
Length of output: 4886
🏁 Script executed:
#!/bin/bash
# Check if there are any comments or documentation in the crates/pyaugurs directory
fd -t f -e md -e txt crates/pyaugursRepository: grafana/augurs
Length of output: 423
🏁 Script executed:
#!/bin/bash
# Search for documentation files in crates/pyaugurs
fd '(README|CONTRIBUTING)' crates/pyaugurs || true
# Also check if there's a CONTRIBUTING or README at project root
fd '(README|CONTRIBUTING)' . -maxdepth 2 | grep -E '\.(md|txt)$' | head -10Repository: grafana/augurs
Length of output: 331
🏁 Script executed:
#!/bin/bash
# Read the README
cat crates/pyaugurs/README.md
echo "---"
# Also check if there are any other justfile targets or setup instructions
grep -n "pyaugurs" justfileRepository: grafana/augurs
Length of output: 2227
🏁 Script executed:
#!/bin/bash
# Check if there are any CI/workflow files that show how tests are run
fd -type f '(\.yml|\.yaml)' . | grep -E '(github|ci|workflow)' | head -5
# Also check if there's a tox.ini or similar
fd -type f '(tox\.ini|Makefile)' .Repository: grafana/augurs
Length of output: 399
🏁 Script executed:
#!/bin/bash
# Search for GitHub workflow files
fd -t f '\.(yml|yaml)$' . | grep -i 'workflow\|github' | head -10
# Also check .github directory
ls -la .github/workflows/ 2>/dev/null | head -20 || echo "No .github/workflows found"Repository: grafana/augurs
Length of output: 588
🏁 Script executed:
#!/bin/bash
# Read the pytest workflow
echo "=== pytest.yml ==="
cat .github/workflows/pytest.yml
echo -e "\n=== python.yml ==="
cat .github/workflows/python.ymlRepository: grafana/augurs
Length of output: 7824
🏁 Script executed:
#!/bin/bash
# Let's check the uv documentation or see if there are comments about this pattern
# Also verify the exact behavior by checking if there's additional context
# Check if there's a specific command that should be used
rg -n "uv sync|uv pip install" crates/pyaugurs --type txt --type md --type tomlRepository: grafana/augurs
Length of output: 144
Based on my verification, I have found critical evidence that confirms the review comment's concern.
Key Finding: The CI workflow (.github/workflows/pytest.yml) uses a fundamentally different approach than the justfile:
- CI workflow: Uses
uv sync --project crates/pyaugurs --all-extras --devto install all dependencies including dev dependencies (pytest) - Justfile: Uses
uv pip install -e . --force-reinstall --no-depswhich skips all dependencies
The --no-deps flag prevents installation of both runtime dependencies (numpy) and dev dependencies (pytest). When uv run pytest is subsequently called, pytest will not be available in the environment, causing the test target to fail.
The justfile build-pyaugurs and test-pyaugurs targets need to be updated to install dev dependencies. Replace uv pip install -e . --force-reinstall --no-deps with uv sync --project crates/pyaugurs --all-extras --dev, or add an additional step to install dev dependencies before running tests. Without this change, the test-pyaugurs target will fail because pytest and numpy won't be available.
🤖 Prompt for AI Agents
In justfile around lines 56 to 62, the build-pyaugurs target uses "uv pip
install -e . --force-reinstall --no-deps" which skips installing runtime and dev
deps (so pytest and numpy are missing), causing test-pyaugurs to fail; update
build-pyaugurs to install dev dependencies by replacing the uv pip install
command with "uv sync --project crates/pyaugurs --all-extras --dev" (or add an
extra step that runs uv sync --project crates/pyaugurs --all-extras --dev before
running tests) so the environment contains pytest and other required
dependencies before calling uv run pytest.
Expanded Python test coverage from 2 basic DTW tests to 120 comprehensive tests covering all major functionality.
1 participant
Expanded Python test coverage from 2 basic DTW tests to 120 comprehensive
tests covering all major functionality.
Summary by CodeRabbit
Breaking Changes
epstoepsilonin the public API and messages.Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.