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

[MNT] PEP 508 environment markers for estimators #6144

Merged
merged 17 commits into from Apr 26, 2024
Merged

[MNT] PEP 508 environment markers for estimators #6144

merged 17 commits into from Apr 26, 2024

Conversation

fkiraly
Copy link
Collaborator

@fkiraly fkiraly commented Mar 17, 2024

This PR adds functionality for adding PEP 508 environment markers for specifying estimator dependencies.

This is useful to specify operating systems with which the estimator is not compatible, e.g., platform_system!="Windows".
The information is added as a string-valued tag env_marker, which must be PEP 508 compliant.

The logic is tied into _check_estimator_deps, so estimators marked with operating system excludes will not be tested on the respective operating system, and will raise a user friendly error message pointing to its requirements.

Minor change: moved default values for all current packaging tags related to the env up to the BaseObject. Later we could move this into scikit-base.

FYI @ianspektor, @achoum, @yarnabrina, this would allow us to release the temporian transformer with a tag that excludes windows operating systems, and we can delay the fix of the windows related issue to after the next sktime release. What do you think?

@fkiraly fkiraly added maintenance Continuous integration, unit testing & package distribution enhancement Adding new functionality module:base-framework BaseObject, registry, base framework labels Mar 17, 2024
Copy link
Collaborator

@yarnabrina yarnabrina left a comment

Choose a reason for hiding this comment

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

I like the idea, but not exactly following how will we use it. Can you please share some details?

sktime/base/_base.py Outdated Show resolved Hide resolved
sktime/utils/validation/_dependencies.py Outdated Show resolved Hide resolved
@fkiraly
Copy link
Collaborator Author

fkiraly commented Mar 17, 2024

I like the idea, but not exactly following how will we use it. Can you please share some details?

Situation: suppose an estimator is not compatible with a specific OS, e.g., windows. Like the current temporian one.

Solution: add the tag env_marker='platform_sys!="Windows"'. This solves the issue since (a) the estimator is not tested on windows VM, and (b) windows users will receive a message that this estimator is not compatible with windows.

@ianspektor
Copy link
Contributor

This is awesome @fkiraly, thank you. Would love to get the transformer merged w/o it depending on the windows fix (that we will address asap anyway).

@yarnabrina
Copy link
Collaborator

yarnabrina commented Mar 18, 2024

I like the idea, but not exactly following how will we use it. Can you please share some details?

Situation: suppose an estimator is not compatible with a specific OS, e.g., windows. Like the current temporian one.

Solution: add the tag env_marker='platform_sys!="Windows"'. This solves the issue since (a) the estimator is not tested on windows VM, and (b) windows users will receive a message that this estimator is not compatible with windows.

Understood. Yes it makes sense, but just wondering if it's enough to add these markers in the pyproject.toml or not.

For example, suppose we add a marker for temporian that windows is not supported as of now with the upper bound version that we use. Then with an estimator tag users can not use thia transformer on windows until next release of sktime, even if @ianspektor fixes the issue and makes a new release of temporian.

In a lot cases we have seen that dependabot does not behave as expected or completely fail to detect new versions, plus even if it does users won't get it until one more month for next sktime patch/minor release. But if we add the tag just in pyproject.toml, CI will be happy in the meanwhile and users can start using on temporian if a new version is released with the fix.

(Just to be clear, I'm not opposing the tag, I'm just trying to think of possible strict restrictions that may be imposed on users if a check is added to disablean estimator on a OS based on these tags. If it's just for informing users that it's ot well tested on this OS, then I've no feedbacks.)

@fkiraly
Copy link
Collaborator Author

fkiraly commented Mar 18, 2024

I see - perhaps it's better than if we add the temporian restriction in pyproject for the current release, and discuss how we manage estimator level dependencies, separately?

For temporian, using pyproject for now makes sense, since there is only a single dependent object/estimator.

@ianspektor
Copy link
Contributor

We're fine with whatever makes it easy for you guys 👍🏼 I see you release ~once a month so even if it's not ideal I wouldn't worry too much about users having to wait for an sktime release once we've fixed the issue (just my 2 cents, I think the arguments for going with the pyproject make sense).

@fkiraly fkiraly merged commit 6c7f6ec into main Apr 26, 2024
204 of 231 checks passed
@fkiraly fkiraly deleted the env-markers branch April 26, 2024 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adding new functionality maintenance Continuous integration, unit testing & package distribution module:base-framework BaseObject, registry, base framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants