Relax UMAP version constraint - #7850
Merged
Merged
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
github-actions
Bot
added
conda
csadorf
added
improvement
This reverts commit 0e0ad73.
… conditions for 'haversine', 'wminkowski', and 'mahalanobis' metrics to ensure proper data dimensionality.
…d version and adjust cloning logic. Remove compatibility checks for scikit-learn 1.8 and clean up xfail-list.yaml by removing obsolete entries.
Contributor
Author
|
/ok to test 2966218 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughRelaxed umap-learn pins to >=0.5.7,<0.5.12 across configs; added runtime branching in UMAP overrides for changed parameter names; removed several sklearn-version test skips and FutureWarning filters; enhanced xfail condition parsing and made upstream UMAP test checkout/tagging dynamic. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cuml/cuml_accel_tests/test_core.py`:
- Line 3: The test file currently only does a top-level "import importlib" but
later calls importlib.metadata.version("umap-learn") (around the call at line
where importlib.metadata.version is invoked), which can raise AttributeError on
Python 3.11; add an explicit "import importlib.metadata" near the other imports
at the top of the file so importlib.metadata is guaranteed to be present before
the importlib.metadata.version(...) call.
In `@python/cuml/cuml_accel_tests/upstream/umap/run-tests.sh`:
- Around line 15-16: UMAP tag construction assumes the "release-" prefix for all
versions; update the logic around UMAP_VERSION/UMAP_TAG to handle older tags by
either (a) checking if the "release-${UMAP_VERSION}" tag exists before using it
and falling back to "${UMAP_VERSION}" if not, or (b) using a version check
(UMAP_VERSION >= 0.5.5) to decide whether to prefix "release-"; ensure the
downstream git checkout uses the resolved tag and that the existence check
(e.g., git tag or git rev-parse) is used to avoid a failed git checkout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cf81750c-ea6b-443e-b03a-8f20ea7f1535
📒 Files selected for processing (14)
conda/environments/all_cuda-129_arch-aarch64.yamlconda/environments/all_cuda-129_arch-x86_64.yamlconda/environments/all_cuda-131_arch-aarch64.yamlconda/environments/all_cuda-131_arch-x86_64.yamldependencies.yamlpython/cuml/cuml/accel/_overrides/umap.pypython/cuml/cuml_accel_tests/integration/test_umap.pypython/cuml/cuml_accel_tests/test_core.pypython/cuml/cuml_accel_tests/upstream/umap/run-tests.shpython/cuml/cuml_accel_tests/upstream/umap/xfail-list.yamlpython/cuml/pyproject.tomlpython/cuml/tests/test_sklearn_import_export.pypython/cuml/tests/test_trustworthiness.pypython/cuml/tests/test_umap.py
💤 Files with no reviewable changes (4)
- python/cuml/cuml_accel_tests/upstream/umap/xfail-list.yaml
- python/cuml/tests/test_sklearn_import_export.py
- python/cuml/tests/test_umap.py
- python/cuml/tests/test_trustworthiness.py
jameslamb
approved these changes
Mar 5, 2026
jameslamb
left a comment
jameslamb
left a comment
Member
There was a problem hiding this comment.
Some minor comments, but approving so you don't need another review from me after addressing them.
dantegd
requested changes
Mar 5, 2026
dantegd
left a comment
dantegd
left a comment
Contributor
There was a problem hiding this comment.
lgtm, just a request to update docs
Technically not necessary since the module would not be imported if uamp was not available, but this is ok as a defensive measure.
dantegd
approved these changes
Mar 5, 2026
dantegd
approved these changes
Mar 5, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cuml/cuml_accel_tests/test_core.py`:
- Around line 89-90: The xfail reason text is inconsistent with the actual gate
predicate in the test: the code uses
Version(importlib.metadata.version("umap-learn")) < Version("0.5.8") but the
reason string says "<= 0.5.7"; update the reason to match the predicate (e.g.,
change the xfail reason to state "umap-learn < 0.5.8" or "< 0.5.8") so the
message aligns with the Version(...) < Version("0.5.8") check in the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: de7a1cf0-5604-402f-b54b-e475f5949669
📒 Files selected for processing (4)
docs/source/supported_versions.rstpython/cuml/cuml/accel/_overrides/umap.pypython/cuml/cuml_accel_tests/test_core.pypython/cuml/cuml_accel_tests/upstream/umap/xfail-list.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- python/cuml/cuml_accel_tests/upstream/umap/xfail-list.yaml
There was a problem hiding this comment.
🧹 Nitpick comments (2)
python/cuml/cuml/accel/pytest_plugin.py (1)
46-61: Narrow the exception scope in condition evaluation.At line 60,
except Exceptioncan hide unexpected defects and silently disable xfail conditions by returningFalse. Catch only the exceptions that can actually be raised:InvalidRequirement(from malformed requirement strings) andPackageNotFoundError(when a package is not installed).Proposed diff
-from importlib.metadata import version +from importlib.metadata import PackageNotFoundError, version ... -from packaging.requirements import Requirement +from packaging.requirements import InvalidRequirement, Requirement ... - except Exception: + except (InvalidRequirement, PackageNotFoundError): return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/accel/pytest_plugin.py` around lines 46 - 61, The _evaluate_single_condition function currently swallows all exceptions; replace the broad `except Exception` with a narrowed exception handler that only catches malformed requirement and missing-package errors: catch `InvalidRequirement` (from packaging.requirements) and `PackageNotFoundError` (from importlib.metadata or importlib_metadata depending on project imports) and return False in that handler, letting other exceptions propagate; keep the rest of the logic using `Requirement(condition_str.strip())`, `version(req.name)`, and `req.specifier.contains(...)` unchanged.python/cuml/cuml_accel_tests/upstream/xfail_manager.py (1)
361-368: UseInvalidRequirementinstead of catching all exceptions.The broad
except Exception as e:at line 364 can mask unrelated runtime issues. SinceRequirement(clause.strip())only raisesInvalidRequirementfor parsing errors, narrow the exception handler to specifically catch that documented exception frompackaging.requirements.Proposed diff
-from packaging.requirements import Requirement +from packaging.requirements import InvalidRequirement, Requirement ... - except Exception as e: + except InvalidRequirement as e: errors.append( f"Group {i} has invalid condition " f"'{group.condition}': {e}" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml_accel_tests/upstream/xfail_manager.py` around lines 361 - 368, The except block that currently catches all exceptions when parsing each clause with Requirement(clause.strip()) should be narrowed to catch packaging.requirements.InvalidRequirement only; replace "except Exception as e:" with "except InvalidRequirement as e:" (import InvalidRequirement from packaging.requirements if not already imported) so only parse errors append the message to errors for Group i using group.condition, leaving other runtime exceptions to propagate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@python/cuml/cuml_accel_tests/upstream/xfail_manager.py`:
- Around line 361-368: The except block that currently catches all exceptions
when parsing each clause with Requirement(clause.strip()) should be narrowed to
catch packaging.requirements.InvalidRequirement only; replace "except Exception
as e:" with "except InvalidRequirement as e:" (import InvalidRequirement from
packaging.requirements if not already imported) so only parse errors append the
message to errors for Group i using group.condition, leaving other runtime
exceptions to propagate.
In `@python/cuml/cuml/accel/pytest_plugin.py`:
- Around line 46-61: The _evaluate_single_condition function currently swallows
all exceptions; replace the broad `except Exception` with a narrowed exception
handler that only catches malformed requirement and missing-package errors:
catch `InvalidRequirement` (from packaging.requirements) and
`PackageNotFoundError` (from importlib.metadata or importlib_metadata depending
on project imports) and return False in that handler, letting other exceptions
propagate; keep the rest of the logic using
`Requirement(condition_str.strip())`, `version(req.name)`, and
`req.specifier.contains(...)` unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bc641d31-32a7-4506-9bd3-799ae3a871d2
📒 Files selected for processing (4)
python/cuml/cuml/accel/pytest_plugin.pypython/cuml/cuml_accel_tests/upstream/README.mdpython/cuml/cuml_accel_tests/upstream/umap/xfail-list.yamlpython/cuml/cuml_accel_tests/upstream/xfail_manager.py
✅ Files skipped from review due to trivial changes (1)
- python/cuml/cuml_accel_tests/upstream/README.md
Contributor
Author
|
/merge |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #7848