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

Add TorchData as hard dependency #1985

Merged
merged 4 commits into from
Nov 21, 2022
Merged

Conversation

ejguan
Copy link
Contributor

@ejguan ejguan commented Nov 11, 2022

Per title

  • Add install_torchdata.sh as the pre-script to install torchdata and specify TORCHDATA_VERSION and CONDA_TORCHDATA_CONSTRAINT in BUILD_ENV_FILE

Copy link
Contributor Author

@ejguan ejguan left a comment

Choose a reason for hiding this comment

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

I added TORCHDATA_VERSION against all the places with PYTORCH_VERSION. But, I am not sure how build is going to use it for torchtext releases.

@ejguan
Copy link
Contributor Author

ejguan commented Nov 14, 2022

@Nayef211 @osalpekar
I did some research on the build system when I tried to enable torchdata as the hard dependency. I think there are a few requirements from test-infra to make torchdata as a hard dependency. When building torchtext, torch is installed from link. If I want to install torchdata via pre-script, due to pre-script only allows a file path, I don't have a way to know if the channel is nightly or test, and I have to add different scripts for conda or wheel and hardcoded the pre-script per build workflow. Besides, I am not able to specify the version of torchdata during the step of building torchtext binary.

In order to resolve this problem, I personally think it would be good to run source "${BUILD_ENV_FILE}" in the step of pre-script.

@osalpekar
Copy link
Member

osalpekar commented Nov 14, 2022

@ejguan Thanks for the investigation - we can certainly source ${BUILD_ENV_FILE} before running the pre/post scripts. I'll try to put up a PR for this later today.

@osalpekar
Copy link
Member

@ejguan PR is up: pytorch/test-infra#1073. Will monitor builds to ensure no existing builds are broken, but otherwise feel free to review/experiment with.

@ejguan ejguan force-pushed the add_torchdata_dep branch 6 times, most recently from 77a3d8b to 72d9b58 Compare November 16, 2022 16:43
@ejguan ejguan marked this pull request as ready for review November 16, 2022 18:00
@ejguan
Copy link
Contributor Author

ejguan commented Nov 16, 2022

I think the failed test is unrelated to this PR. Edit: All tests passed

Copy link
Contributor

@Nayef211 Nayef211 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks so much for taking care of adding the torchdata dependency @ejguan!

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.

Add TorchData as a required dependency in TorchText for 0.13.0 release
4 participants