Skip to content

Conversation

@gaogaotiantian
Copy link
Contributor

What changes were proposed in this pull request?

Upgrade numpy version on lint image and fixed some minor lint failures.

Why are the changes needed?

When we do pip install ./dev/requirements.txt locally, we normally have the latest version of numpy. This creates a diff between our local dev environment and CI. We should keep this as close as possible so we can rely on local mypy results.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Locally mypy test passed.

Was this patch authored or co-authored using generative AI tooling?

No

@github-actions
Copy link

JIRA Issue Information

=== Improvement SPARK-55132 ===
Summary: Upgrade numpy version on lint image
Assignee: None
Status: Open
Affected: ["4.2.0"]


This comment was automatically generated by GitHub Actions

@Yicong-Huang
Copy link
Contributor

In this case, would it be better to unpin the version so docker file picks up the latest version by default?

@gaogaotiantian
Copy link
Contributor Author

I think that's the same dilemma for pinning ruff version. If lint image picks up the latest numpy version and reports an error on all PRs around that time, who's responsible to fix it? However we did not pin scipy or pandas. So I guess we are kind of just doing it arbitrarily.

Copy link
Contributor

@zhengruifeng zhengruifeng left a comment

Choose a reason for hiding this comment

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

LGTM pending CI

BTW, we may also want to upgrade the python version here, have a try if you are interested

@gaogaotiantian
Copy link
Contributor Author

Sure we can upgrade to 3.12. I think ideally we should do python lint check on all supported versions - but that's low priority. We don't have to do it on different workflows. We should be able to quickly switch between versions and do checks.

@zhengruifeng
Copy link
Contributor

thanks, merged to master

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants