Install 3rd party libraries in CI for lint by not522 · Pull Request #3580 · optuna/optuna

not522 · 2022-05-23T09:36:32Z

Motivation

In the checks workflow, we have not installed 3rd party libraries for visualization, integration, benchmarks, etc. It skips some checks, especially in mypy.

Description of the changes

Install 3rd party libraries in CI for lint.

not522 · 2022-05-25T10:32:53Z

pytest.warns(None) is deprecated. https://docs.pytest.org/en/7.0.x/deprecations.html#using-pytest-warns-none

codecov-commenter · 2022-05-25T10:57:16Z

Codecov Report

Merging #3580 (24cfda9) into master (e8b6922) will increase coverage by 0.00%.
The diff coverage is 19.04%.

@@           Coverage Diff           @@
##           master    #3580   +/-   ##
=======================================
  Coverage   90.56%   90.57%           
=======================================
  Files         164      164           
  Lines       12646    12652    +6     
=======================================
+ Hits        11453    11459    +6     
  Misses       1193     1193

Impacted Files	Coverage Δ
optuna/integration/pytorch_distributed.py	`36.87% <0.00%> (ø)`
optuna/integration/pytorch_lightning.py	`0.00% <0.00%> (ø)`
optuna/integration/xgboost.py	`59.67% <0.00%> (ø)`
optuna/integration/chainer.py	`89.74% <80.00%> (-0.26%)`	⬇️
optuna/samplers/_tpe/parzen_estimator.py	`98.29% <0.00%> (+0.04%)`	⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

not522 · 2022-05-26T01:08:42Z

optuna/integration/pytorch_lightning.py

    def on_init_start(self, trainer: Trainer) -> None:
-        self.is_ddp_backend = trainer._accelerator_connector.distributed_backend is not None
+        self.is_ddp_backend = (
+            trainer._accelerator_connector.distributed_backend is not None  # type: ignore


distributed_backend has been removed. Lightning-AI/pytorch-lightning#10017
See also #3418.

not522 · 2022-05-26T01:10:08Z

Checks CI passed. Now, this PR is ready for review.

contramundum53 · 2022-05-26T09:02:20Z

@toshihikoyanase Could you review this PR?

contramundum53

LGTM!

toshihikoyanase

Thank you for your PR. The type checking in Python is getting more and more popular, and it may be a good time to introduce more strict checking. On the other hand, I'm not confident with the change in terms of developers' experience.

The change will increase the duration of the checks jobs from 1 min to 15 min. I see some developers use the jobs instead of a local linter and their turn-around time will also increase. Also, the required CI jobs might be more fragile to the update of third-party libraries.

Not a strong opinion, but how about adding this strict checks job to daily CI?
Currently, the daily CI is mainly checked by the committers and it hardly affects external developers.

github-actions · 2022-06-07T23:05:25Z

This pull request has not seen any recent activity.

toshihikoyanase · 2022-06-08T01:05:12Z

I talked with @not522 , and they will move this strict lint to daily CI. Keep this PR as it is, please.

github-actions · 2022-06-15T23:05:23Z

This pull request has not seen any recent activity.

toshihikoyanase · 2022-06-20T12:10:03Z

If we install recent xgboost, xgb.core.CallbackEnv is not defined, and the following errors are raised. So we need type: ignore there.

optuna/integration/xgboost.py:79: error: Name "xgb.core.CallbackEnv" is not defined
optuna/integration/xgboost.py:100: error: Name "xgb.core.CallbackEnv" is not defined

https://github.com/optuna/optuna/runs/6552734319?check_suite_focus=true

On the other hand, the errors will not be raised when we do not install xgboost since we ignore missing import.

optuna/integration/xgboost.py:7[9](https://github.com/optuna/optuna/runs/6912328489?check_suite_focus=true#step:8:10): error: Unused "type: ignore" comment
optuna/integration/xgboost.py:100: error: Unused "type: ignore" comment

https://github.com/optuna/optuna/runs/6912328489?check_suite_focus=true

IMO, we can omit the type check of older xgboost, for example, by using @typing.no_type_check. (But I'm not sure if it is expected usage of the annotation). And, I'm also wondering why we keep the callback since the other integration modules only support the latest version of the target libraries.

This reverts commit d4bb34d.

not522 · 2022-07-01T06:48:40Z

I found following errors.

xarray.Dataset.from_dict is an untyped function. (It will be fixed in the next xarray release.)
chainer's and pytorch.distributed's functions are untyped.
The latest PyTorchLightning is not supported.
The latest XGBoost is not supported.
Incompatible subclassing in test_pytorch_lightning.py.
pytest.warns(None) is deprecated.

For 1, we need to wait.
2 is hard to resolve. I added type: ignore comments.
3 and 4 should be fixed in other PRs.
5 is hard to resolve. I recommend that we just ignore it.
6 should be fixed in this PR.

not522 · 2022-07-08T07:58:58Z

.github/workflows/checks-integration.yml

+        python -m pip install -U pip
+        pip install --progress-bar off -U .[checking]
+        pip install --progress-bar off -U .[optional]
+        pip install --progress-bar off -U .[integration] --extra-index-url https://download.pytorch.org/whl/cpu
+        pip install --progress-bar off -U .[benchmark]
+        pip install --progress-bar off -U bayesmark
+        pip install --progress-bar off -U kurobako


Skip installing naslib because it requires specific library versions.

not522 · 2022-07-08T08:00:02Z

All checks are passed. PTAL.

toshihikoyanase · 2022-07-12T05:02:53Z

setup.cfg

 check_untyped_defs = True
 no_implicit_optional = True
 warn_redundant_casts = True
-warn_unused_ignores = True


[Notes] .github/workflows/checks-integration.yml checks with this option (warn_unused_ignores) and unnecessary # type: ignore will be detected there.

toshihikoyanase

Thank you for your update. The change looks great to me.
Some changes are not intuitive to me, and I left some notes just for reminder.

toshihikoyanase · 2022-07-13T07:46:15Z

tests/integration_tests/test_pytorch_lightning.py

+        accuracy = sum(
+            x["validation_accuracy"] for x in cast(List[Dict[str, torch.Tensor]], outputs)
+        ) / len(outputs)


[Notes] outputs can be List[torch.Tensor], but original code apparently expects List[Dict[str, torch.Tensor]] , so this fix is acceptable to me.

toshihikoyanase · 2022-07-13T07:52:52Z

.github/workflows/checks-integration.yml

+    steps:
+    - uses: actions/checkout@v2


The scheduled run might not be intended to run in forks. But, we can work on it in a follow-up PR.

optuna/.github/workflows/speed-benchmarks.yml

Lines 11 to 12 in fc5e262

# Not intended for forks.

if: github.repository == 'optuna/optuna'

toshihikoyanase · 2022-07-14T04:21:07Z

@not522 requested another review from @contramundum53 . Please merge after your review.

contramundum53

LGTM. Thanks!

not522 added 4 commits May 23, 2022 18:30

Install 3rd party libraries for lint

0d8a3ee

Add setup environment step

a89a597

ignore type checking for xgboost 1.6

9675feb

mypy for PyTorch Lightning integration

b1e52b2

not522 force-pushed the lint-3rd-party branch from d7d9b5f to b1e52b2 Compare May 25, 2022 10:05

Use pytest.warns() instead of pytest.warns(None)

1401aea

mypy for PyTorch Lightning 1.6.0

b9bcaf1

not522 commented May 26, 2022

View reviewed changes

not522 marked this pull request as ready for review May 26, 2022 01:10

contramundum53 assigned toshihikoyanase May 26, 2022

contramundum53 approved these changes May 26, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/master' into lint-3rd-party

7d1c4f8

toshihikoyanase reviewed May 27, 2022

View reviewed changes

toshihikoyanase added the CI Continuous integration. label May 31, 2022

github-actions bot added the stale Exempt from stale bot labeling. label Jun 7, 2022

github-actions bot removed the stale Exempt from stale bot labeling. label Jun 8, 2022

toshihikoyanase mentioned this pull request Jun 15, 2022

Enable strict_equality for mypy #3579 #3648

Merged

github-actions bot added the stale Exempt from stale bot labeling. label Jun 15, 2022

not522 added 2 commits June 16, 2022 10:09

Add daily lint workflow with 3rd party libraries

45827cb

Merge remote-tracking branch 'upstream/master' into lint-3rd-party

fdc9c53

contramundum53 removed the stale Exempt from stale bot labeling. label Jun 20, 2022

Enable checks-integration temporary

c44295f

not522 added 8 commits June 28, 2022 10:17

Install kurobako and NASLib

d4bb34d

Fix call to untyped function

d4bc537

Remove redundant cast

bef1717

Revert "Install kurobako and NASLib"

a2b3bf9

This reverts commit d4bb34d.

Install kurobako

3a4b95f

Ignore typing error for patches

68307b2

Merge branch 'master' into lint-3rd-party

2f982e3

Ignore incompatible subclassing

1479033

Merge remote-tracking branch 'upstream/master' into lint-3rd-party

ecfbab5

not522 force-pushed the lint-3rd-party branch from 5f82357 to 67567d3 Compare July 8, 2022 06:50

Add manual trigger for checks-integration workflow

f91d7e5

not522 force-pushed the lint-3rd-party branch from 67567d3 to f91d7e5 Compare July 8, 2022 06:52

Stop checks-integration on PR

24cfda9

not522 marked this pull request as ready for review July 8, 2022 07:55

not522 commented Jul 8, 2022

View reviewed changes

not522 requested a review from contramundum53 July 8, 2022 08:00

toshihikoyanase reviewed Jul 12, 2022

View reviewed changes

toshihikoyanase approved these changes Jul 13, 2022

View reviewed changes

toshihikoyanase reviewed Jul 13, 2022

View reviewed changes

toshihikoyanase assigned contramundum53 and unassigned toshihikoyanase Jul 14, 2022

contramundum53 approved these changes Jul 14, 2022

View reviewed changes

contramundum53 added this to the v3.0.0-rc0 milestone Jul 14, 2022

contramundum53 merged commit 1f9dd70 into optuna:master Jul 14, 2022

not522 deleted the lint-3rd-party branch August 19, 2022 10:53

c-bata mentioned this pull request Nov 16, 2022

Trigger checks-integration action on PR creation and run mypy only. #4181

Closed

not522 mentioned this pull request Apr 4, 2025

Optional mypy check #6028

Merged

	# Not intended for forks.
	if: github.repository == 'optuna/optuna'

Uh oh!

Conversation

not522 commented May 23, 2022

Motivation

Description of the changes

Uh oh!

not522 commented May 25, 2022

Uh oh!

codecov-commenter commented May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

not522 May 26, 2022

Choose a reason for hiding this comment

Uh oh!

not522 commented May 26, 2022

Uh oh!

contramundum53 commented May 26, 2022

Uh oh!

contramundum53 left a comment

Choose a reason for hiding this comment

Uh oh!

toshihikoyanase left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 7, 2022

Uh oh!

toshihikoyanase commented Jun 8, 2022

Uh oh!

github-actions bot commented Jun 15, 2022

Uh oh!

toshihikoyanase commented Jun 20, 2022

Uh oh!

not522 commented Jul 1, 2022

Uh oh!

not522 Jul 8, 2022

Choose a reason for hiding this comment

Uh oh!

not522 commented Jul 8, 2022

Uh oh!

toshihikoyanase Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

toshihikoyanase left a comment

Choose a reason for hiding this comment

Uh oh!

toshihikoyanase Jul 13, 2022

Choose a reason for hiding this comment

Uh oh!

toshihikoyanase Jul 13, 2022

Choose a reason for hiding this comment

Uh oh!

toshihikoyanase commented Jul 14, 2022

Uh oh!

contramundum53 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented May 25, 2022 •

edited

Loading