-
Notifications
You must be signed in to change notification settings - Fork 6.8k
remove windows note for dataloader, fix deprecated warning in python3.7 #11650
Conversation
Great! Is this covered by tests or do we maybe have a skip defined somewhere? |
the fixed deprecation is on python 3.7 which is not supported in CI. I think we should start supporting py37 soon. |
Definitely! My question is whether we test the data loader on windows. Sometimes, we have some conditional skips if we run on windows. It would be great if we could verify that this test is actually run on windows. |
Good point, let me go through the old tests and figure it out |
@marcoabreu This is behavioral test, the test will crash if not supported. |
Okay, can we then remove the print to reduce the log output and replace it with some no-op? We're already having a lot of useless log (stuff like a function printing the numbers from 1 to 10000) and I'd like to keep that low in future if the log messages don't provide any information about the test itself. |
print('{}:{}'.format(epoch, i)) | ||
# sanity tests | ||
# enable test on windows | ||
data = Dummy(False) |
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.
Did you intentionally set the randomized shape to false? It used to be true before.
Also, could elaborate on the comment in line 210? Is there anything specific we have to do on Windows or is it working out of the box? If it doesn't need any special configuration, we can safely remove the comment as it is otherwise confusing to people who don't know the history
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 am deliberately switch one test case to fixed shape, and the rest one is responsible for the random test.
- Will remove the unnecessary comments
BTW, my PR disappeared in CI yesterday but show up today, do you have a clue?
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 see, thanks for elaborating.
Yeah, we had some disk space problems on our server. Everything should be fine now :)
close due to duplicate efforts in #11908 |
Description
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Comments