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

Fix numpy 2.0 e2e test fail with lower requirements #1949

Merged
merged 23 commits into from
Jun 21, 2024

Conversation

ravi-kumar-pilla
Copy link
Contributor

@ravi-kumar-pilla ravi-kumar-pilla commented Jun 18, 2024

Description

Resolves https://github.com/kedro-org/kedro-viz/actions/runs/9548598519/job/26316143362?pr=1935

 Full exception: `np.float_` was removed in the NumPy 2.0 release. Use `np.float64` instead.                  

Development notes

  • Pin numpy in package/features/steps/lower_requirements.txt at 1.26.4
  • Pin numpy in package/features/steps/cli_steps.py at 1.26.4
  • Doc link fix at docs/source/publish_and_share_kedro_viz_on_azure.md

QA notes

  • All tests should pass

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

Signed-off-by: ravi-kumar-pilla <[email protected]>
Signed-off-by: ravi-kumar-pilla <[email protected]>
Signed-off-by: ravi-kumar-pilla <[email protected]>
Signed-off-by: ravi-kumar-pilla <[email protected]>
Signed-off-by: ravi-kumar-pilla <[email protected]>
Signed-off-by: ravi-kumar-pilla <[email protected]>
Signed-off-by: ravi-kumar-pilla <[email protected]>
@ravi-kumar-pilla
Copy link
Contributor Author

Hi Team,

I am trying to fix e2e test failures on kedro-viz. For both the cases below, pinning numpy < 2 resolved the issue. But I am not certain to go ahead with this fix as the actual fix might be in the starters. Could someone please confirm if the starters are working fine with latest release of numpy.

https://github.com/kedro-org/kedro-viz/actions/runs/9548599222/job/26316144191?pr=1935
https://github.com/kedro-org/kedro-viz/actions/runs/9571094566/job/26387337026

NOTE: Kedro-Viz itself does not use numpy but seems like pandas uses numpy. Compatibility matrix mentioned here might help.

cc: @ankatiyar @SajidAlamQB

Thank you

@ankatiyar
Copy link
Contributor

ankatiyar commented Jun 19, 2024

It makes sense to cap numpy in lower_requirements.txt for this failure - https://github.com/kedro-org/kedro-viz/actions/runs/9548599222/job/26316144191?pr=1935
Since pandas==1.15 does not have an upper bound on numpy

But why does it need to be pinned in package/requirements.txt? What commit is this failure related to - https://github.com/kedro-org/kedro-viz/actions/runs/9571094566/job/26387337026? It seems like a missing dependency from kedro-datasets 👀 @ravi-kumar-pilla

@ravi-kumar-pilla
Copy link
Contributor Author

But why does it need to be pinned in package/requirements.txt? What commit is this failure related to - https://github.com/kedro-org/kedro-viz/actions/runs/9571094566/job/26387337026? It seems like a missing dependency from kedro-datasets 👀 @ravi-kumar-pilla

Hi @ankatiyar , Thank you for looking over the PR. This is not related to any new commit. Our builds were failing all of a sudden without any code change (even the old builds if re-run are failing). The e2e test that fails now is for the below scenario -

Scenario: Execute viz with the earliest Kedro version that it supports 
    Given I have installed kedro version "0.18.3"
    And I have run a non-interactive kedro new with pandas-iris starter
    And I have installed the project's requirements
    When I execute the kedro viz run command
    Then kedro-viz should start successfully

It might be some issue with the pandas-iris starter requirements as Kedro-Viz works fine with the demo-project we have. I will change the starter for the test and try again. But I wanted to bring this to your attention if we need to push changes to the starters due to the numpy release.

Thank you

@ankatiyar
Copy link
Contributor

Okay I created a project with 0.18.3 and tried it and I see what is happening -
For this particular test, when a Kedro project's requirements are installed, it does pandas==1.15.x which installs numpy==2.0.0 because it's not capped. It's the same problem but I don't think we should pin the numpy version in package/requirements.txt for all of kedro-viz but only for this specific e2e test.

I don't think anything of Kedro side is breaking since we run our tests only with the latest versions of pandas which works fine with the latest numpy.

Signed-off-by: ravi-kumar-pilla <[email protected]>
@@ -80,6 +80,15 @@ def install_project_requirements(context):
"""Run ``pip install -r requirements.txt``."""
if context.kedro_version != "latest":
requirements_path = str(context.root_project_dir) + "/src/requirements.txt"

with open(requirements_path, "r") as req_file:
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also add a comment here that this is because numpy 2.0 breaks with old versions of pandas and this could be removed when the lowest version supported is updated

package/features/steps/lower_requirements.txt Show resolved Hide resolved
RELEASE.md Outdated
- Display published URLs. (#1907)

## Bug fixes and other changes

- Fix numpy 2.0 e2e test fail with lower requirements. (#1949)
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't need to be in the release notes I think as it's not a user facing change!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure on this. We do mention changes in the - ## Bug fixes and other changes section which are not user facing (like - - Migrate from CircleCi to GitHub Actions. (#1876)). But will remove it for this PR. Need to know what to mention in this section of the release note.

Signed-off-by: ravi-kumar-pilla <[email protected]>
Signed-off-by: ravi-kumar-pilla <[email protected]>
@ravi-kumar-pilla ravi-kumar-pilla marked this pull request as ready for review June 20, 2024 15:46
@ravi-kumar-pilla ravi-kumar-pilla requested review from ankatiyar and Huongg and removed request for rashidakanchwala June 20, 2024 15:47
Signed-off-by: ravi-kumar-pilla <[email protected]>
Copy link
Contributor

@ankatiyar ankatiyar left a comment

Choose a reason for hiding this comment

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

💯

Copy link
Contributor

@Huongg Huongg left a comment

Choose a reason for hiding this comment

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

thanks @ravi-kumar-pilla, looking good

@ravi-kumar-pilla ravi-kumar-pilla merged commit 3af6144 into main Jun 21, 2024
26 checks passed
@ravi-kumar-pilla ravi-kumar-pilla deleted the fix/numpy2-requirements branch June 21, 2024 00:03
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.

3 participants