-
Notifications
You must be signed in to change notification settings - Fork 813
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
use 'cpu' instead of None for Iterator #745
Conversation
@@ -64,7 +65,7 @@ def __init__(self, dataset, batch_size, sort_key=None, device=None, | |||
logger.warning("The `device` argument should be set by using `torch.device`" | |||
+ " or passing a string as an argument. This behavior will be" | |||
+ " deprecated soon and currently defaults to cpu.") | |||
device = None | |||
device = torch.device('cpu') |
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.
It makes sense that the default device is cpu if device is None. But if device
is provided in the init func, you still force to use cpu. Seem not correct. And double check the doc explains this clearly.
Should I change accordingly or leave it as it is?? |
If device is provided in the |
I will change accordingly |
Made changes as per your comments |
torchtext/data/iterator.py
Outdated
@@ -65,7 +65,8 @@ def __init__(self, dataset, batch_size, sort_key=None, device=None, | |||
logger.warning("The `device` argument should be set by using `torch.device`" | |||
+ " or passing a string as an argument. This behavior will be" | |||
+ " deprecated soon and currently defaults to cpu.") | |||
device = torch.device('cpu') |
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 not correct. The device
is under if isinstance
statement. For None
device, it's no way to enter it.
You have to rebase to the master branch |
changed as per your comments |
The CI tests fail. |
i fixed indentation issue |
The circleci tests need to pass. Please check the log since the errors are relevant. |
Fixed |
nit: could you remove the parentheses in Python conditional? |
Done!! |
@@ -65,6 +66,10 @@ def __init__(self, dataset, batch_size, sort_key=None, device=None, | |||
+ " or passing a string as an argument. This behavior will be" | |||
+ " deprecated soon and currently defaults to cpu.") | |||
device = None | |||
|
|||
if device is None: | |||
device = torch.device('cpu') |
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.
In the doc, device could also be string. Should we also cover that as:
if device is None:
device = torch.device('cpu')
elif isinstance(device, str):
device = torch.device(device)
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 would also like to add one more thing. How about adding a warning if cuda is available and user still preferred 'cpu'
if torch.cuda.is_available():
if device is None or device=='cpu':
logger.warning("you have gpu available but preferred device as cpu")
if device is None:
device = torch.device('cpu')
elif isinstance(device, str):
device = torch.device(device)
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 wouldn't assume that GPU is preferred if it is available. This might be used in some unexpected context.
Alongside updating the default behavior we should also update the doc string to document this.
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.
Sometimes users forget to pass 'gpu' parameter even if gpu is available. So, if we send a warning, users can rectify it.
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.
Yes, but it adds verbosity we commonly don't have. I suggest we do this in a separate PR where we address logging issues in general. If we were to do it for this operator we should also do it for the other ones and that'll require some strategy.
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.
ok then, for now i will ignore and add only required ones.
@zhangguanheng66 please merge this |
@avinashsai - The docstring wasn't updated. Please send a follow-up PR. |
but it should be |
@cpuhrsch where should I edit the docs? Iam not able to find the location. |
The default is |
Fixes #376
Points default device to torch cpu instead of None if no device is specified to be compatible with torch device types