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

Retire legacy code in torchtext #3529

Closed
zhangguanheng66 opened this issue Sep 17, 2020 · 17 comments
Closed

Retire legacy code in torchtext #3529

zhangguanheng66 opened this issue Sep 17, 2020 · 17 comments
Labels
design Includes a design discussion feature Is an improvement or enhancement let's do it! approved to implement priority: 1 Medium priority task
Milestone

Comments

@zhangguanheng66
Copy link

❓ Questions and Help

As the maintainer of the torchtext library, we plan to retire torchtext.data.batch.Batch and torchtext.data.example.Example in the next release (by the end of October). Those two components will be still available in torchtext.legacy. To handle the compatibility issue, should we send a PR and fix this on GitHub first, or we could just make some changes in fbcode?

Here are the proposed changes:

# old API
from torchtext.data.batch import Batch
from torchtext.data.example import Example

# new API
from torchtext.legacy.data.batch import Batch
from torchtext.legacy.data.example import Example

Relevant issue: pytorch/text#985

@zhangguanheng66 zhangguanheng66 added the question Further information is requested label Sep 17, 2020
@github-actions
Copy link
Contributor

Hi! thanks for your contribution!, great first issue!

@Borda Borda added design Includes a design discussion feature Is an improvement or enhancement and removed question Further information is requested labels Sep 18, 2020
@Borda
Copy link
Member

Borda commented Sep 18, 2020

I think when you know from which version you are going to change it we can implement it right away on one of the following ways (also if you have another proposal it is welcome)

  1. with known version:
    if torchtext.__version__ < X.Y:
        from torchtext.data.batch import Batch
        from torchtext.data.example import Example
    else:
        from torchtext.legacy.data.batch import Batch
        from torchtext.legacy.data.example import Example
    
  2. generic ways:
    try:
        from torchtext.legacy.data.batch import Batch
        from torchtext.legacy.data.example import Example
    except ImportError:
        from torchtext.data.batch import Batch
        from torchtext.data.example import Example
    

@Borda Borda added the let's do it! approved to implement label Sep 18, 2020
@zhangguanheng66
Copy link
Author

I think when you know from which version you are going to change it we can implement it right away on one of the following ways (also if you have another proposal it is welcome)

  1. with known version:
    if torchtext.__version__ < X.Y:
        from torchtext.data.batch import Batch
        from torchtext.data.example import Example
    else:
        from torchtext.legacy.data.batch import Batch
        from torchtext.legacy.data.example import Example
    
  2. generic ways:
    try:
        from torchtext.legacy.data.batch import Batch
        from torchtext.legacy.data.example import Example
    except ImportError:
        from torchtext.data.batch import Batch
        from torchtext.data.example import Example
    

Thanks. We will know which the version we will change. I'm just wondering if I should change this on Github first, or I can send a Diff internally to fbcode and later on sync with the master branch. I just want to know the mechanism to sync PyTorchLightening between fbcode and github.

@Borda
Copy link
Member

Borda commented Sep 18, 2020

you can prepare a PR and later ping us when it actually, just not to forget it...
even throw we would find it as soon as you release it that some our tests are failing :]

@zhangguanheng66
Copy link
Author

Sounds good. For the timeline, we will drop those legacy code from the master branch early October and eventually in 0.8.0 release (10/27/2020).

@Borda
Copy link
Member

Borda commented Sep 18, 2020

so I think that if you decided to go with version check you can send it now...

@ananthsub
Copy link
Contributor

Thanks. We will know which the version we will change. I'm just wondering if I should change this on Github first, or I can send a Diff internally to fbcode and later on sync with the master branch. I just want to know the mechanism to sync PyTorchLightening between fbcode and github.

@zhangguanheng66 There is none. PyTorch Lightning isn't linked to internal pytorch

@zhangguanheng66
Copy link
Author

Thanks. We will know which the version we will change. I'm just wondering if I should change this on Github first, or I can send a Diff internally to fbcode and later on sync with the master branch. I just want to know the mechanism to sync PyTorchLightening between fbcode and github.

@zhangguanheng66 There is none. PyTorch Lightning isn't linked to internal pytorch

@ananthsub Thanks. So I have to manually fix the internal version by myself, right? Is there anyone importing OSS PyTorch Lightning to fbcode periodically?

@ananthsub
Copy link
Contributor

ananthsub commented Sep 20, 2020

That’s me + our team. We have scripts to do this and update whenever there are new releases. I’ll ping you to follow up

@stale
Copy link

stale bot commented Oct 21, 2020

This issue has been automatically marked as stale because it hasn't had any recent activity. This issue will be closed in 7 days if no further activity occurs. Thank you for your contributions, Pytorch Lightning Team!

@stale stale bot added the won't fix This will not be worked on label Oct 21, 2020
@Borda
Copy link
Member

Borda commented Oct 21, 2020

@zhangguanheng66 what is the progress here?

@stale stale bot removed the won't fix This will not be worked on label Oct 21, 2020
@zhangguanheng66
Copy link
Author

@zhangguanheng66 what is the progress here?

We are still cleaning up the new building blocks in torchtext so we postponed the retirement of the legacy code (maybe Nov. and December).

@stale
Copy link

stale bot commented Nov 20, 2020

This issue has been automatically marked as stale because it hasn't had any recent activity. This issue will be closed in 7 days if no further activity occurs. Thank you for your contributions, Pytorch Lightning Team!

@stale stale bot added the won't fix This will not be worked on label Nov 20, 2020
@Borda Borda added this to the 1.1 milestone Nov 20, 2020
@Borda Borda removed the won't fix This will not be worked on label Nov 20, 2020
@Borda Borda modified the milestones: 1.1, 1.1.x Nov 30, 2020
@Borda
Copy link
Member

Borda commented Dec 13, 2020

@zhangguanheng66 what is the progress here?

We are still cleaning up the new building blocks in torchtext so we postponed the retirement of the legacy code (maybe Nov. and December).

@zhangguanheng66 any update here, the freeze for <0.7 becoming a blocker for us for an upgrade on python 3.9 #4944 as the first torch supporting it is torch>=1.7.1 and so your torchtext==0.8.1

@Borda Borda added the priority: 1 Medium priority task label Dec 13, 2020
@Borda Borda modified the milestones: 1.1.x, 1.2 Dec 20, 2020
@zhangguanheng66
Copy link
Author

For now, you can use 0.8.1 for torchtext, although the legacy code is still there.

@edenlightning edenlightning modified the milestones: 1.2, 1.3 Feb 8, 2021
@zhangguanheng66
Copy link
Author

zhangguanheng66 commented Feb 18, 2021

We have landed the PR to retire the legacy code pytorch/text#1172. This will be released in torchtext v0.9.0, and we are targeting the release date on March 4.

Similar changes have been done in fbcode.

If you still prefer to use batch, you can add the legacy tag to the path, a.k.a.

from torchtext.legacy.data.batch import Batch

@Borda Borda modified the milestones: 1.3, 1.2 Feb 18, 2021
@Borda
Copy link
Member

Borda commented Mar 2, 2021

@Borda Borda mentioned this issue Mar 2, 2021
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Includes a design discussion feature Is an improvement or enhancement let's do it! approved to implement priority: 1 Medium priority task
Projects
None yet
Development

No branches or pull requests

4 participants