-
Notifications
You must be signed in to change notification settings - Fork 419
Refactor: third phase of RESTRUCTURE.md #2361
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
Conversation
cfcb929 to
d89814e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @SamuelMarks. Generally LGTM, just a few comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you locally test building some of the Docker images, running setup.sh, other relevant tests, etc.? I can test the benchmark_runner tomorrow with a Docker image. Maybe pip installing from source also (although I don't think this should be impacted)
d89814e to
3551eb2
Compare
63dac06 to
ee58f3d
Compare
d6ddb7b to
ab8458f
Compare
ab8458f to
6c08332
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to split this PR into two PRs? One for dependencies and one for tools
Also, we will want to run manual tests for this one
f1309e9 to
785b692
Compare
53be2d2 to
189e938
Compare
0a6267f to
10a067e
Compare
10a067e to
b4e08bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM just one comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, please put the benchmark runner test result in the PR description as well before merging
Description
Third phase of RESTRUCTURE.md. Follows #2144 & #2304.
Note: merge this after #2541; which roughly halves this PR.
mkdir -p dependencies/{dockerfiles,requirements,scripts} tools/{data_generation,dev,gcs_benchmarks,orchestration,setup,weight_inspector} while read -r f; do mv "$f" dependencies/dockerfiles/; done< <(gfind . \( -type d -name .git -prune -not -name .git \) -o -type f -name '*Dockerfile') for name in 'jetstream_pathways' 'maxengine_server'; do git checkout main ./src/MaxText/inference/"$name"/Dockerfile mv ./src/MaxText/inference/"$name"/Dockerfile ./dependencies/dockerfiles/"$name".Dockerfile done rm dependencies/dockerfiles/Dockerfile mv *.txt dependencies/requirements/ mv {docker_build_dependency_image,docker_upload_runner}.sh dependencies/scripts/ mv {download_dataset.sh,src/MaxText/generate_distillation_data.py} tools/data_generation/ mv {code_style.sh,unit_test_and_lint.sh} tools/dev/ mv src/MaxText/standalone_{checkpointer,dataloader}.py tools/gcs_benchmarks/ mv {gpu_multi_process_run.sh,multihost_job.py,multihost_runner.py} tools/orchestration/ mv setup*sh tools/setup/ mv src/MaxText/weight_inspector.py tools/weight_inspector/ for d in data_generation gcs_benchmarks orchestration weight_inspector; do cp src/MaxText/experimental/__init__.py tools/"$d"; done cp src/MaxText/experimental/__init__.py toolsTests
Whence CI passes manual testing can happen; then this is ready for merge.
This worked on a TPU VM:
Checklist
Before submitting this PR, please make sure (put X in square brackets):