Remove unnecessary flake8 exceptions #3762
Conversation
There was a problem hiding this comment.
Unfortunately we need to have F401: unused imports for tests as a side effect of how Registrable works - in our tests which do things from Params, we need to import the classes we need for the individual tests so that they are registered by the time we try to construct them from params.
Also - i'm slightly dis-inclined with the reshuffling of imports in this PR, some of them seem not necessary?
|
@DeNeutoy, just using On reshuffling of imports: when I touch a file, I will often sort the imports in that file - makes it easier to find things later. I didn't look closely at all of the shuffling in this PR, but it looks like that's all that's going on here. |
|
Oh ok, happy to take your lead on both issues |
I'm trying to add the missing imports. The
I think we can add a linter to optimize the imports, like in the transformers library: https://github.com/huggingface/transformers/blob/ee5de0ba449d638da704e1c03ffcc20a930f5589/.circleci/config.yml#L105 |
.flake8
Outdated
| */__init__.py:F401 | ||
| */**/**/__init__.py:F401 |
There was a problem hiding this comment.
Apparently this works. If you can come up with a way to grab all with one line, that'd be cool.
There was a problem hiding this comment.
Please leave the lines-too-long; black will catch anything that's fixable, so we don't need flake to complain at us for these files. Even if there aren't any files where it matters now, it'd be annoying to have this come up later.
| with warnings.catch_warnings(): | ||
| warnings.filterwarnings("ignore", category=FutureWarning) | ||
| import h5py |
There was a problem hiding this comment.
I put it at the end so the regular imports are sorted.
| @@ -434,7 +434,7 @@ def push_python_path(path: PathType) -> ContextManagerFunctionReturnType[None]: | |||
| sys.path.remove(path) | |||
|
|
|||
|
|
|||
| def import_submodules(package_name: str) -> None: | |||
| def import_module_and_submodules(package_name: str) -> None: | |||
There was a problem hiding this comment.
Note I make this change mentioned in another PR here.
| @@ -217,7 +217,7 @@ def read(fn: str) -> Iterable[List[Extraction]]: | |||
| yield prev_sent | |||
|
|
|||
|
|
|||
| def convert_sent_to_conll(sent_ls: List[Extraction]) -> Iterable[Tuple[str, str, ...]]: | |||
| def convert_sent_to_conll(sent_ls: List[Extraction]) -> Iterable[Tuple[str, ...]]: | |||
There was a problem hiding this comment.
I changed this because in the end you can only put ellipsis in the second arg.
|
Tests pass now, but I'm not quite sure where what was missing gets imported exactly. Also, I believe that tests that load archives with config files can locally import what's necessary inside the test function (or |
.flake8
Outdated
| */__init__.py:F401 | ||
| */**/**/__init__.py:F401 |
There was a problem hiding this comment.
Please leave the lines-too-long; black will catch anything that's fixable, so we don't need flake to complain at us for these files. Even if there aren't any files where it matters now, it'd be annoying to have this come up later.
|
About messing with |
|
It may not be necessary, but I and others have run into it enough times that it's convenient to have, and it doesn't cause problems. |
* Create nightly.yml * check diff from 24 hr ago, use environment for version.py * get things, not call * add 24 hr diff script * typo * add dev, check for tests to pass * santiago's suggested change Co-Authored-By: Santiago Castro <[email protected]> * fix flake from #3762 * change time to 11 Co-authored-by: Santiago Castro <[email protected]>
* example for feedback * remove all existing multiprocessing * sneak torch datasets inside DatasetReader * lint * trainer_v2, We Love To See It * datasets have index_with now, not iterators * use iter, custom collate function in allennlp wrapper * we don't even need the data in the trainer anymore * all trainer tests passing * black * make find learning rate work * update test fixtures to new config * get train command tests mostly working * lazily construct samplers, index lazy datasets * update some fixtures * evaluate tests passing * all command tests passing * lint * update model test case, common and module tests passing * fix test interdependence introduced by #3762 * more test interdependence * tests tests tests * remove unnecessary brackets Co-Authored-By: Santiago Castro <[email protected]> * update a chunk of the configs * fix archival test, couple more configs * rm pointless gan test * more tests passing * add current state of from params changes * Revert "add current state of from params changes" This reverts commit ad45659. * updated understanding of Lazy * add discussion of None comparison to Lazy * lint * it's a hard doc life * pull samplers into separate file * more docs updates * fold in #3812 * remove torch dataset * add example to lazy * rename to collate * no kwargs * Revert "fold in #3812" This reverts commit 8a08899. * don't break up dataset * add comment to iterable dataset len * improve docstrings, build dataloader using partial_objects * flake * give dataloader a default implementation * safer default for DataLoader init * more coherent dir structure * update imports * add a test for the BucketBatchSampler * split bucket sampler into own file, tests * PR comments Co-authored-by: Santiago Castro <[email protected]>
Also tackles #3760 (comment) and #3759 (comment)