Add retire warning to legacy code #863
Conversation
zhangguanheng66
requested review from
Nayef211,
cpuhrsch,
fmassa,
mthrok and
vincentqb
Codecov Report
@@ Coverage Diff @@
## master #863 +/- ##
==========================================
+ Coverage 77.35% 77.43% +0.08%
==========================================
Files 43 43
Lines 3029 3045 +16
==========================================
+ Hits 2343 2358 +15
- Misses 686 687 +1
Continue to review full report at Codecov.
|
torchtext/data/batch.py
Outdated
|
|
||
| def __init__(self, data=None, dataset=None, device=None): | ||
| """Create a Batch from a list of examples.""" | ||
| warnings.warn('Batch class will retire in 0.8.0 release', RuntimeWarning) |
There was a problem hiding this comment.
According to this comment, PyTorch's convention for deprecation is UserWarning.
There was a problem hiding this comment.
FYI: torchaudio has depricated decorator which can be used for both function and a class.
torchtext/data/example.py
Outdated
| """ | ||
| @classmethod | ||
| def fromJSON(cls, data, fields): | ||
| warnings.warn('Example class will retire in 0.8.0 release', RuntimeWarning) |
There was a problem hiding this comment.
For all of these warnings, the user could ask themselves many questions. Is there any context that should be given? What is the user to do instead? Should the user file an issue if they can't do what they want?
There was a problem hiding this comment.
We have an issue to explain this change. I might add this to the warning message. And in 0.7.0 release note, I will introduce this again.
There was a problem hiding this comment.
Would adding the link in the message be a good idea? btw can you share the link here in the comment? :)
There was a problem hiding this comment.
Based on offline discussion: Please use a standard message for all. The message needs to say: (1) users are recommended to move to the new interface by doing abc, but (2) they can keep using the same functionality without the warning by doing xyz.
zhangguanheng66
force-pushed
the
legacy_warning
branch
from
29e18d9 to
f58cec4
Compare
torchtext/data/batch.py
Outdated
|
|
||
| def __init__(self, data=None, dataset=None, device=None): | ||
| """Create a Batch from a list of examples.""" | ||
| warnings.warn('Batch class will retire in 0.8.0 release and stay in torchtext.legacy. See 0.7.0 release note for the replacement.', UserWarning) |
There was a problem hiding this comment.
Can you add the link to the release notes?
There was a problem hiding this comment.
For now, we don't have a link. And I'm thinking if it's proper to add a link in warning message.
If we put URL on message, there is a high chance that certain user codebase outlive the URL itself, so I do not think adding URL is a good idea. It's better to have a self-complete message. IIRC the discussion, those code will be moved to torchtext.legacy module, right? Can we just say "Use torchtext.legacy"?
There was a problem hiding this comment.
That's a good question, but I'm also not a fan of mentioning the release notes alone :)
torchtext/data/example.py
Outdated
| """ | ||
| @classmethod | ||
| def fromJSON(cls, data, fields): | ||
| warnings.warn('Example class will retire in 0.8.0 release and stay in torchtext.legacy. See 0.7.0 release note for the replacement.', UserWarning) |
There was a problem hiding this comment.
I'd change the phrasing of this a bit.
"Example class will retire in 0.8.0 release and stay in torchtext.legacy. See 0.7.0 release note for the replacement"
to
"Example class will be retired in the 0.8.0 release and moved to torchtext.legacy. Please see 0.7.0 release notes for further information."
There was a problem hiding this comment.
Did we settle on UserWarning? cc @gchanan
There was a problem hiding this comment.
With some offline discussions, UserWarning is proper, though pytorch started to use FutureWarning.
torchtext/data/example.py
Outdated
| """ | ||
| @classmethod | ||
| def fromJSON(cls, data, fields): | ||
| warnings.warn('Example class will be retired in the 0.8.0 release and moved to torchtext.legacy. Please see 0.7.0 release note for further information.', UserWarning) |
There was a problem hiding this comment.
nit: It's "notes" not "note".
4 participants
No description provided.