Skip to content

Optional mypy check#6028

Merged
nabenabe0928 merged 15 commits intooptuna:masterfrom
Alnusjaponica:optional-mypy-check
Apr 23, 2025
Merged

Optional mypy check#6028
nabenabe0928 merged 15 commits intooptuna:masterfrom
Alnusjaponica:optional-mypy-check

Conversation

@Alnusjaponica
Copy link
Copy Markdown
Contributor

@Alnusjaponica Alnusjaponica commented Apr 1, 2025

Motivation

Keep lint & type check passed with optional dependency.

Description of the changes

This PR adds a daily CI to check if lint & type check with optional dependency and resolve current problems.

@Alnusjaponica Alnusjaponica mentioned this pull request Apr 1, 2025
Closed
@Alnusjaponica
Copy link
Copy Markdown
Contributor Author

Alnusjaponica commented Apr 1, 2025 •
edited
Loading

See Alnusjaponica#31 for the actual result.

@Alnusjaponica Alnusjaponica marked this pull request as ready for review April 1, 2025 06:51
@nabenabe0928
Copy link
Copy Markdown
Contributor

nabenabe0928 commented Apr 2, 2025

@not522 Could you review this PR?

nabenabe0928
nabenabe0928 reviewed Apr 2, 2025
View reviewed changes
nabenabe0928
nabenabe0928 reviewed Apr 2, 2025
View reviewed changes
nabenabe0928
nabenabe0928 reviewed Apr 2, 2025
View reviewed changes
nabenabe0928
nabenabe0928 reviewed Apr 2, 2025
View reviewed changes
nabenabe0928
nabenabe0928 reviewed Apr 2, 2025
View reviewed changes
nabenabe0928
nabenabe0928 reviewed Apr 2, 2025
View reviewed changes
@Alnusjaponica @nabenabe0928
Rename CI
fa501cb 
Co-authored-by: Shuhei Watanabe <47781922+nabenabe0928@users.noreply.github.com>
not522
not522 reviewed Apr 2, 2025
View reviewed changes
@HideakiImamura
Copy link
Copy Markdown
Member

HideakiImamura commented Apr 3, 2025

@sawa3030 Could you review this PR?

@not522
Copy link
Copy Markdown
Member

not522 commented Apr 4, 2025

Note for the context of this PR: Checks with optional dependencies was introduced in #3580 and removed in #5277.

@not522
Copy link
Copy Markdown
Member

not522 commented Apr 9, 2025

Could you remove unused "type: ignore" comments? https://github.com/not522/optuna/actions/runs/14347148737/job/40218995477

@sawa3030 sawa3030 mentioned this pull request Apr 15, 2025
Closed
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 15, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 95.83333% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.36%. Comparing base (11ff024) to head (0458a7c).
Report is 24 commits behind head on master.

Files with missing lines Patch % Lines
optuna/storages/_rdb/models.py 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #6028      +/-   ##
==========================================
- Coverage   88.39%   88.36%   -0.03%     
==========================================
  Files         206      206              
  Lines       13921    13916       -5     
==========================================
- Hits        12305    12297       -8     
- Misses       1616     1619       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nabenabe0928 nabenabe0928 added the code-fix Change that does not change the behavior, such as code refactoring. label Apr 16, 2025
@nabenabe0928 nabenabe0928 added this to the v4.4.0 milestone Apr 16, 2025
@not522
Copy link
Copy Markdown
Member

not522 commented Apr 22, 2025

Could you fix the CI error?

nabenabe0928
nabenabe0928 reviewed Apr 22, 2025
View reviewed changes
nabenabe0928
nabenabe0928 reviewed Apr 23, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@nabenabe0928 nabenabe0928 left a comment

Choose a reason for hiding this comment

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

Almost LGTM once the following is addressed:

@Alnusjaponica @nabenabe0928
type: ignore[assignment] for _Column
e4a717d 
Co-authored-by: Shuhei Watanabe <47781922+nabenabe0928@users.noreply.github.com>
@Alnusjaponica
Copy link
Copy Markdown
Contributor Author

Alnusjaponica commented Apr 23, 2025

Sorry for my delayed reply. I've applied the suggested changes and watching Alnusjaponica#32 to see the result

nabenabe0928
nabenabe0928 approved these changes Apr 23, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@nabenabe0928 nabenabe0928 left a comment

Choose a reason for hiding this comment

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

Thank you for the update, LGTM!

@nabenabe0928 nabenabe0928 removed their assignment Apr 23, 2025
not522
not522 approved these changes Apr 23, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@not522 not522 left a comment

Choose a reason for hiding this comment

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

LGTM!

@nabenabe0928 nabenabe0928 merged commit 0e509f7 into optuna:master Apr 23, 2025
15 checks passed
@Alnusjaponica Alnusjaponica deleted the optional-mypy-check branch April 23, 2025 16:24
@kvr06-ai kvr06-ai mentioned this pull request Mar 10, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sawa3030 sawa3030 sawa3030 left review comments

@not522 not522 not522 approved these changes

+1 more reviewer

@nabenabe0928 nabenabe0928 nabenabe0928 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@not522 not522

Labels

code-fix Change that does not change the behavior, such as code refactoring.

Projects

None yet

Milestone

v4.4.0

Development

Successfully merging this pull request may close these issues.

5 participants

@Alnusjaponica @nabenabe0928 @HideakiImamura @not522 @sawa3030