Skip to content

chore: Allow numpy >= 2.0.0#479

Closed
booxter wants to merge 1 commit into
mainfrom
ihrachyshka-numpy
Closed

chore: Allow numpy >= 2.0.0#479
booxter wants to merge 1 commit into
mainfrom
ihrachyshka-numpy

Conversation

@booxter

@booxter booxter commented Apr 18, 2025

Copy link
Copy Markdown
Contributor

A large e2e run with forced numpy 2.0+ suggests that the library is compatible with numpy 2.x series.

Also, removed numpy dependency from requirements-dev.txt because it's already listed in requirements.txt.

See: https://github.com/instructlab/training/actions/runs/14539292904

Signed-off-by: Ihar Hrachyshka ihar.hrachyshka@gmail.com

@booxter booxter marked this pull request as draft April 18, 2025 17:19
@mergify mergify Bot added CI/CD Affects CI/CD configuration ci-failure dependencies Pull requests that update a dependency file labels Apr 18, 2025
@github-actions

Copy link
Copy Markdown

E2E (NVIDIA L40S x4) workflow launched on this PR: View run

@github-actions

Copy link
Copy Markdown

e2e workflow succeeded on this PR: View run, congrats!

@booxter booxter force-pushed the ihrachyshka-numpy branch from a9bae5c to 5b27dd6 Compare April 18, 2025 22:05
@booxter booxter changed the title Test: bump numpy to 2+ chore: Allow numpy >= 2.0.0 Apr 18, 2025
@booxter booxter marked this pull request as ready for review April 18, 2025 22:05
@booxter booxter force-pushed the ihrachyshka-numpy branch from 5b27dd6 to 4612bca Compare April 19, 2025 01:14
@mergify mergify Bot added ci-failure and removed ci-failure labels Apr 19, 2025
@RobotSail

Copy link
Copy Markdown
Member

@booxter We use numpy in a few critical points like the collator function in the dataloader as well as the data processing logic when converting from messages into raw input IDs for the model to consume. This is a PR where we'd need to run training and validate that the loss curve from your changes produces the same loss curve as what's in the main branch.

@booxter

booxter commented Apr 28, 2025

Copy link
Copy Markdown
Contributor Author

Fair enough. We'll need to automate validation for patches like this one (also refactors etc.) I don't feel we should continue with human assessments (or at the very least artifacts - charts etc. - needed for such assessment should be generated and posted in comments.) I will mark this as a draft and report an issue to implement the necessary automation.

@booxter booxter marked this pull request as draft April 28, 2025 20:15
@RobotSail

Copy link
Copy Markdown
Member

@booxter Yes, we have a script to do this already but I think it broke and I haven't had time to look into it. If you can fix it then that would be a huge contribution which would automate most of the testing we do manually today. You can find it here: https://github.com/instructlab/training/blob/main/scripts/create-loss-graph.py

And then the actual CI code to call it can be found here:

python training/scripts/create-loss-graph.py \

@booxter

booxter commented May 1, 2025

Copy link
Copy Markdown
Contributor Author

@Mergifyio rebase

@mergify

mergify Bot commented May 1, 2025

Copy link
Copy Markdown
Contributor

rebase

✅ Branch has been successfully rebased

@booxter booxter force-pushed the ihrachyshka-numpy branch from 4612bca to 37a41c3 Compare May 1, 2025 14:22
@mergify mergify Bot removed the ci-failure label May 1, 2025
@booxter booxter force-pushed the ihrachyshka-numpy branch from 37a41c3 to 09366f6 Compare June 4, 2025 15:16
@booxter

booxter commented Jun 4, 2025

Copy link
Copy Markdown
Contributor Author

Running e2e here again with enforced numpy2: https://github.com/instructlab/training/actions/runs/15446098645

@github-actions

github-actions Bot commented Jun 4, 2025

Copy link
Copy Markdown

E2E (NVIDIA L40S x4) workflow launched on this PR: View run

@booxter

booxter commented Jun 4, 2025

Copy link
Copy Markdown
Contributor Author

@mergify

mergify Bot commented Jun 4, 2025

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. @booxter please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label Jun 4, 2025
A large e2e run with forced numpy 2.0+ suggests that the library is
compatible with numpy 2.x series.

Also, removed `numpy` dependency from requirements-dev.txt because it's
already listed in requirements.txt.

See: https://github.com/instructlab/training/actions/runs/14539292904

Signed-off-by: Ihar Hrachyshka <ihar.hrachyshka@gmail.com>
@booxter booxter force-pushed the ihrachyshka-numpy branch from 09366f6 to 545e705 Compare June 4, 2025 18:08
@mergify mergify Bot removed the needs-rebase label Jun 4, 2025
@github-actions

github-actions Bot commented Jun 4, 2025

Copy link
Copy Markdown

e2e workflow succeeded on this PR: View run, congrats!

@booxter

booxter commented Jun 4, 2025

Copy link
Copy Markdown
Contributor Author

Looking at charts, visually, I don't see any difference:

with numpy2: https://github.com/instructlab/training/actions/runs/15446098645
with numpy1: https://github.com/instructlab/training/actions/runs/15443591418

@booxter booxter marked this pull request as ready for review June 4, 2025 19:47
@booxter

booxter commented Jun 4, 2025

Copy link
Copy Markdown
Contributor Author

Except visual comparison, is there any additional prerequisite to hit before merging this? @RobotSail

@booxter booxter closed this Jun 23, 2025
@RobotSail

Copy link
Copy Markdown
Member

@booxter Sorry I didn't see your message prior as I was out on PTO. Ideally we just need to check what APIs we are using from numpy and whether any are impacted. 95% likely it's fine but would be good to double-check.

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

Labels

CI/CD Affects CI/CD configuration dependencies Pull requests that update a dependency file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants