Skip to content

[BUG] use instance-level tags in _check_estimator_deps - #601

Open
Me-Priyank wants to merge 1 commit into
sktime:mainfrom
Me-Priyank:bug/check-estimator-deps-instance-tags
Open

Me-Priyank wants to merge 1 commit into
sktime:mainfrom
Me-Priyank:bug/check-estimator-deps-instance-tags

Conversation

@Me-Priyank

Copy link
Copy Markdown

Reference Issues/PRs

Fixes #600. See also sktime/sktime#10520, sktime/sktime#10528.

What does this implement/fix? Explain your changes.

_check_estimator_deps, _check_python_version and _check_env_marker read the python_dependencies, python_version and env_marker tags via get_class_tag, so dynamic tag overrides on instances were ignored.

This PR adds a private helper _get_tag in skbase/utils/dependencies/_dependencies.py, used in all three places:

  • for instances, tags are read via get_tag(..., raise_error=False), i.e., including dynamic tag overrides;
  • for classes, tags are read via get_class_tag, unchanged. Objects without get_tag also fall back to get_class_tag, as before.

Also:

  • updated the obj docstrings of the three functions accordingly, and corrected the obj description of _check_env_marker (previously "used to check python version");
  • registered _get_tag in SKBASE_FUNCTIONS_BY_MODULE in skbase/tests/conftest.py;
  • added test_check_estimator_deps_dynamic_tags, parametrized over the three tags. It checks both directions: a failing class tag cleared on the instance passes, and a failing tag set only on the instance fails. The test fails on main and passes with this change.

Does your contribution introduce a new dependency? If yes, which one?

No.

What should a reviewer concentrate their feedback on?

  • the helper semantics: instances use get_tag, classes and objects without get_tag use get_class_tag
  • downstream effect on sktime once it allows a release containing this:

Any other comments?

Local checks: full skbase test suite incl. doctests (1620 passed, 23 skipped), skbase/_nopytest_tests.py, and all pre-commit hooks on the changed files pass.

PR checklist

For all contributions
  • I've reviewed the project documentation on contributing
  • I've added myself to the list of contributors.
  • The PR title starts with either [ENH], [CI/CD], [MNT], [DOC], or [BUG] indicating whether
    the PR topic is related to enhancement, CI/CD, maintenance, documentation, or a bug.
For code contributions
  • Unit tests have been added covering code functionality
  • Appropriate docstrings have been added (see documentation standards)

Dependency checks now respect dynamic tag overrides when given an instance.
@Me-Priyank

Copy link
Copy Markdown
Author

@fkiraly PTAL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] _check_estimator_deps ignores instance-level tags, so dynamic dependency tags (e.g. sktime ignore_deps=True) are not respected

1 participant