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: function name 'requires_agumentation' to 'requires_augmentation' #118

Merged
merged 4 commits into from
Jan 9, 2025

Conversation

willmj
Copy link
Collaborator

@willmj willmj commented Jan 6, 2025

Nit fix for function name. Would require a change in sft_trainer.py as well if this is wanted.

@willmj willmj requested a review from fabianlim as a code owner January 6, 2025 16:31
@fabianlim
Copy link
Contributor

fabianlim commented Jan 7, 2025

Hi @willmj thanks for the change looks good. Although it might mean that if this merges then main repo will break correct? I beleive it will affect teh CI processes in the main repo, so we should merge both PRs together. And also if there is a future fms-acceleration release, we need to make a main repo release simultanously? I find this flow abit cumbersome and perhaps can be improved @anhuong

BTW moe plugin is currently not default in image. this is due to dependency on a git repo with the kernels inside. Issue to copy the kernels over is #105

@willmj
Copy link
Collaborator Author

willmj commented Jan 8, 2025

Hey Fabian - that's correct, and as long as we make the changes in both repos and do a fms-acceleration release before fms-hf-tuning, there shouldn't be any problems in the images either.

@willmj
Copy link
Collaborator Author

willmj commented Jan 8, 2025

PR #434 in fms-hf-tuning

@fabianlim
Copy link
Contributor

fabianlim commented Jan 8, 2025

@willmj yea but we did run into that problem that any problems with image build is only caught in the fms-hf CI.. is that too late do you think? should we have a minimal image build CI in fms-accel?

DId you test tha changes of both repos together yet?

@willmj
Copy link
Collaborator Author

willmj commented Jan 8, 2025

I just ran the tests in fms-hf-tuning and some are failing but seem to fail because of other changes not related to this one:

============================================================================================== test session starts ===============================================================================================
platform linux -- Python 3.11.7, pytest-8.3.4, pluggy-1.5.0
rootdir: /app/fms-hf-tuning
configfile: pytest.ini
collected 19 items                                                                                                                                                                                               

tests/acceleration/test_acceleration_framework.py ..s.......FF......F                                                                                                                                      [100%]
============================================================================================ short test summary info =============================================================================================
FAILED tests/acceleration/test_acceleration_framework.py::test_framework_initialized_properly_moe - ValueError: Data loading failed: invalid path None with builder json.
FAILED tests/acceleration/test_acceleration_framework.py::test_framework_initialize_and_trains_with_aadp - ValueError: Data loading failed: invalid path None with builder json.
FAILED tests/acceleration/test_acceleration_framework.py::test_fastkernels_with_full_finetuning_runs_successfully - ValueError: Data loading failed: invalid path None with builder json.
======================================================================== 3 failed, 15 passed, 1 skipped, 13 warnings in 82.38s (0:01:22) =========================================================================

Tests are failing because of the wrong filepath, I will fix in the fms-hf-tuning PR

I don't know if we need an image build for fms-acceleration, we can always limit a version in fms-hf-tuning if a problem arises though.

@willmj
Copy link
Collaborator Author

willmj commented Jan 8, 2025

With this commit all tests pass:

============================================================================================== test session starts ===============================================================================================
platform linux -- Python 3.11.7, pytest-8.3.4, pluggy-1.5.0
rootdir: /app/fms-hf-tuning
configfile: pytest.ini
collected 19 items                                                                                                                                                                                               

tests/acceleration/test_acceleration_framework.py ..s................                                                                                                                                      [100%]

@fabianlim
Copy link
Contributor

ok sounds good @willmj . I merge this PR and then you merge the fms-hf PR?

@willmj
Copy link
Collaborator Author

willmj commented Jan 8, 2025

Yes sounds good @fabianlim, although it needs to be approved again

@fabianlim fabianlim merged commit 03035e6 into foundation-model-stack:main Jan 9, 2025
7 checks passed
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.

2 participants