Skip to content
This repository was archived by the owner on Sep 10, 2025. It is now read-only.

Conversation

@Nayef211
Copy link
Contributor

@Nayef211 Nayef211 commented Aug 30, 2022

Description

@Nayef211
Copy link
Contributor Author

Nayef211 commented Aug 30, 2022

All of the MacOS failures should be fixed by #1886. The Windows unittest failure looks new and will require further investigation but is not caused by this PR.

According to @mthrok, the binary_macos_conda_py3.7 failure happens frequently in torchaudio and is usually ignored by them as they are autoresolved.

@Nayef211 Nayef211 requested review from joecummings and mthrok August 30, 2022 18:27
@Nayef211 Nayef211 marked this pull request as ready for review August 30, 2022 18:28
@Nayef211 Nayef211 changed the title Make symbols in common.h file visible Export symbols in common.h file Aug 30, 2022
Copy link
Member

@joecummings joecummings left a comment

Choose a reason for hiding this comment

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

This looks good, thanks for doing this!

One possible nit: I usually squash (fold in the mercurial world) unimportant commits like "Fixing imports" so there's less clutter in the commit history. If this isn't common practice w/ torchtext, no worries, but thought I'd mention it.

@Nayef211
Copy link
Contributor Author

This looks good, thanks for doing this!

One possible nit: I usually squash (fold in the mercurial world) unimportant commits like "Fixing imports" so there's less clutter in the commit history. If this isn't common practice w/ torchtext, no worries, but thought I'd mention it.

That's a good point. In torchtext, we generally keep all commits (even unimportant ones related to linting), but when we merge we create a PR against the main branch, we always use the "Squash and Merge" option. This way all commits are preserved in the PR/feature branches but commits in the main branch always map 1 to 1 with the corresponding PR. Lmk if this practice makes sense to you, if not we can discuss what the best practice should be moving forward.

@Nayef211 Nayef211 merged commit a37c585 into pytorch:remove-dependency-jit Aug 30, 2022
joecummings added a commit that referenced this pull request Aug 31, 2022
…#1885)

* Remove dependency on the torch::jit::script::Module for mobile builds

Summary: In order to resolve linkage errors. Specifically when vocab getting build for "mobile" version it can't resolve symbols for torch::jit::script::Module

Reviewed By: Nayef211

Differential Revision: D38771271

fbshipit-source-id: 693b656f2a17af9fa5a7a1904742557f902edb55

* Add vocab factory to CMakeLists

* Fix type conversion from py::object to STL container (#1887)

* Export symbols in `common.h` file (#1888)

* Fix type conversion from py::object to STL container

* Adding TORCHTEXT_API to expose symbols in common.h

* Add common.h import in corresponding cpp file

Co-authored-by: Alexander Mazukabzov <[email protected]>
Co-authored-by: Nayef Ahmed <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants