Skip to content

MAINT do not check for warnings in doc_min_dependencies #27949

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

Merged
merged 7 commits into from
Dec 12, 2023

Conversation

glemaitre
Copy link
Member

Solve the issue observed in main where the minimum version of scikit-image is not compatible with python 3.9+

Copy link

github-actions bot commented Dec 12, 2023

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 9d5900f. Link to the linter CI: here

@glemaitre glemaitre changed the title MAINT update minimum version of scikit-image MAINT do not check for warnings in doc_min_dependencies Dec 12, 2023
@glemaitre
Copy link
Member Author

Since bumping scikit-image raise still another warning, I propose to not check for the warning based on an environment variable.

@@ -24,6 +24,7 @@ jobs:
- OPENBLAS_NUM_THREADS: 2
- CONDA_ENV_NAME: testenv
- LOCK_FILE: build_tools/circle/doc_min_dependencies_linux-64_conda.lock
- WARNINGS_AS_ERRORS: 'false'
Copy link
Member

Choose a reason for hiding this comment

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

maybe worth following other environment variable with the SKLEARN_ prefix?

So something like this SKLEARN_DOC_BUILD_WARNINGS_AS_ERRORS to be explicit?

Maybe worth adding to the documented variables here in a separate PR:
https://scikit-learn.org/dev/computing/parallelism.html#environment-variables

In particular this may be worth mentioning that this is different from sphinx-build -W that also turns some warnings into errors.

doc/conf.py Outdated
message="pkg_resources is deprecated as an API",
category=DeprecationWarning,
)
if os.environ.get("WARNINGS_AS_ERRORS", "true") == "true":
Copy link
Member

@lesteve lesteve Dec 12, 2023

Choose a reason for hiding this comment

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

Maybe use .lower() to make sure that True works as well as true

Suggested change
if os.environ.get("WARNINGS_AS_ERRORS", "true") == "true":
if os.environ.get("WARNINGS_AS_ERRORS", "true").lower() == "true":

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

Other than adding a comment for the env variable, LGTM if CI is happy.

@adrinjalali adrinjalali enabled auto-merge (squash) December 12, 2023 17:31
@adrinjalali adrinjalali merged commit a88a33a into scikit-learn:main Dec 12, 2023
message="pkg_resources is deprecated as an API",
category=DeprecationWarning,
)
if os.environ.get("SKLEARN_DOC_BUILD_WARNINGS_AS_ERRORS", "true").lower() == "true":
Copy link
Member

Choose a reason for hiding this comment

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

For info, this is breaking the doc build when using python 3.12 (there is a DeprecationWarning: coming from datetime.datetime.utcfromtimestamp)

Copy link
Member

@lesteve lesteve Jan 17, 2024

Choose a reason for hiding this comment

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

Indeed I think I saw that and left it for later, PR more than welcome 😉! You would need to add them to the already accepted warnings see warnings.filterwarnings("ignore", below

IIRC the deprecation warning is due to dateutil, it has been fixed in main but not released yet, and it seems like it is a bit stuck at the moment dateutil/dateutil#1284 or dateutil/dateutil#1314

glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Feb 10, 2024
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.

4 participants