-
Notifications
You must be signed in to change notification settings - Fork 812
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 binary build #766
Add binary build #766
Conversation
8c5332e
to
aef8e4d
Compare
@zhangguanheng66 I ported binary build job from torchaudio. The benefit of this is that after conda package is built, it performs import test. So the concern regarding C++ extension proper installation is automatically addressed. |
|
||
test: | ||
imports: | ||
- torchtext |
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.
@zhangguanheng66 This is where the import test is defined.
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.
This is the old conda build script with some import tests link
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.
Oh interesting. why was it removed?
jobs: | ||
- circleci_consistency | ||
{{ build_workflows() }} | ||
# - binary_win_conda: |
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 this to be fixed in a follow-up PR?
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.
Not my intent.
I am just keeping what was there and making build_workflows
work.
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.
Should we test the torchtext package with cpp extension on a Windows machine? Although we don't explicitly support Windows, the current package does work on Windows.
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.
Ideally, we should. torchaudio
does not have this but needs this soon.
We can look into how torchvision
does it or ask help from someone with Windows experience.
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.
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.
I'll add unit tests first. Binary builds on Windows will come later.
No description provided.