Skip to content

Conversation

@arnaudgelas
Copy link
Contributor

Rework #5398

import torch
from tqdm import tqdm

import torch
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this correct as torch is external lib?

@codecov
Copy link

codecov bot commented Jan 7, 2021

Codecov Report

Merging #5400 (02ad057) into release/1.2-dev (8a40e80) will not change coverage.
The diff coverage is n/a.

@@               Coverage Diff               @@
##           release/1.2-dev   #5400   +/-   ##
===============================================
  Coverage               93%     93%           
===============================================
  Files                  146     146           
  Lines                10361   10361           
===============================================
  Hits                  9597    9597           
  Misses                 764     764           

Copy link
Contributor

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

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

@arnaudgelas It seems that all the changes in this PR don't comply with our current isort config. You might be accidentally using a different config...?

@arnaudgelas
Copy link
Contributor Author

@akihironitta I don't know why, but on my machine isort considers pandas, torch and torchvision as FIRSTPARTY. If I add a section in pyproject.toml (see added section below), it seems much more coherent.

+known_third_party = [

  • "pandas",
  • "torch",
  • "torchvision"
    +]

Should I proceed?

@akihironitta
Copy link
Contributor

@arnaudgelas That's strange... Which version of isort are you using? I'm using 5.7.0, and it works fine.

@arnaudgelas
Copy link
Contributor Author

arnaudgelas commented Jan 7, 2021

@akihironitta Thanks! You gave me an excellent pointer to fix it. The main problem is pre-commit was relying on isort found on your system. In my case it was an extremely old one (and not the one installed via provided requirements).

see #5408

Note that once the mentioned PR gets merged, I will have to close this one since there won't be anymore error in benchmark folder.

@arnaudgelas arnaudgelas closed this Jan 8, 2021
@akihironitta
Copy link
Contributor

@arnaudgelas You're welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants