Remove unnecessary flake8 exceptions #3762

bryant1410 · 2020-02-11T17:01:41Z

Also tackles #3760 (comment) and #3759 (comment)

DeNeutoy

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?

matt-gardner · 2020-02-11T17:29:36Z

@DeNeutoy, just using AllenNlpTestCase should do the necessary imports, right? I'm not sure why we need the flake exception. There are only 6 failing tests here, and I think they are failing because of missing imports in the corresponding __init__.py files, so this is actually catching bugs. Also, I think we should avoid using from_params to construct things in tests going forward, except for unusual cases, or integration-style tests like our model tests.

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.

DeNeutoy · 2020-02-11T18:04:50Z

Oh ok, happy to take your lead on both issues

bryant1410 · 2020-02-11T21:25:57Z

@DeNeutoy, just using AllenNlpTestCase should do the necessary imports, right? I'm not sure why we need the flake exception. There are only 6 failing tests here, and I think they are failing because of missing imports in the corresponding __init__.py files, so this is actually catching bugs. Also, I think we should avoid using from_params to construct things in tests going forward, except for unusual cases, or integration-style tests like our model tests.

I'm trying to add the missing imports.

The from_params is used in a script. In any case, IIUC, @DeNeutoy you say you're gonna fix this?

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.

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

bryant1410 · 2020-02-11T21:27:31Z

.flake8

+    */__init__.py:F401
+    */**/**/__init__.py:F401


Apparently this works. If you can come up with a way to grab all with one line, that'd be cool.

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.

bryant1410 · 2020-02-11T21:28:00Z

allennlp/commands/elmo.py

+with warnings.catch_warnings():
+    warnings.filterwarnings("ignore", category=FutureWarning)
+    import h5py


I put it at the end so the regular imports are sorted.

bryant1410 · 2020-02-11T21:28:14Z

allennlp/common/util.py

@@ -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:


Note I make this change mentioned in another PR here.

bryant1410 · 2020-02-11T21:29:29Z

scripts/convert_openie_to_conll.py

@@ -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, ...]]:


I changed this because in the end you can only put ellipsis in the second arg.

bryant1410 · 2020-02-11T23:55:27Z

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 setUp).

matt-gardner

LGTM, with one minor change.

matt-gardner · 2020-02-12T04:15:25Z

.flake8

+    */__init__.py:F401
+    */**/**/__init__.py:F401


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.

bryant1410 · 2020-02-12T04:37:21Z

About messing with sys.path within the scripts, is it really necessary? If they're called from the root dir they should work well.

matt-gardner · 2020-02-12T15:14:50Z

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 <sacastro@umich.edu> * fix flake from #3762 * change time to 11 Co-authored-by: Santiago Castro <bryant@montevideo.com.uy>

* 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 <bryant@montevideo.com.uy> * 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 <bryant@montevideo.com.uy>

Remove unnecessary flake8 exceptions

4ff5e9c

bryant1410 requested a review from matt-gardner February 11, 2020 17:01

DeNeutoy reviewed Feb 11, 2020

View reviewed changes

Fix missing import

0eff663

bryant1410 commented Feb 11, 2020

View reviewed changes

matt-gardner approved these changes Feb 12, 2020

View reviewed changes

Fix a PR comment

32db1c3

matt-gardner merged commit c64fb9d into allenai:master Feb 12, 2020

DeNeutoy added a commit that referenced this pull request Feb 12, 2020

fix flake from #3762

aaa4127

DeNeutoy added a commit to DeNeutoy/allennlp that referenced this pull request Feb 21, 2020

fix test interdependence introduced by allenai#3762

ef5187f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove unnecessary flake8 exceptions #3762

Remove unnecessary flake8 exceptions #3762

bryant1410 commented Feb 11, 2020 •

edited

Loading

DeNeutoy left a comment

matt-gardner commented Feb 11, 2020

DeNeutoy commented Feb 11, 2020

bryant1410 commented Feb 11, 2020

bryant1410 Feb 11, 2020

matt-gardner Feb 12, 2020

bryant1410 Feb 11, 2020

bryant1410 Feb 11, 2020

bryant1410 Feb 11, 2020

bryant1410 commented Feb 11, 2020

matt-gardner left a comment •

edited

Loading

matt-gardner Feb 12, 2020

bryant1410 commented Feb 12, 2020

matt-gardner commented Feb 12, 2020

Remove unnecessary flake8 exceptions #3762

Remove unnecessary flake8 exceptions #3762

Conversation

bryant1410 commented Feb 11, 2020 • edited Loading

DeNeutoy left a comment

Choose a reason for hiding this comment

matt-gardner commented Feb 11, 2020

DeNeutoy commented Feb 11, 2020

bryant1410 commented Feb 11, 2020

bryant1410 Feb 11, 2020

Choose a reason for hiding this comment

matt-gardner Feb 12, 2020

Choose a reason for hiding this comment

bryant1410 Feb 11, 2020

Choose a reason for hiding this comment

bryant1410 Feb 11, 2020

Choose a reason for hiding this comment

bryant1410 Feb 11, 2020

Choose a reason for hiding this comment

bryant1410 commented Feb 11, 2020

matt-gardner left a comment • edited Loading

Choose a reason for hiding this comment

matt-gardner Feb 12, 2020

Choose a reason for hiding this comment

bryant1410 commented Feb 12, 2020

matt-gardner commented Feb 12, 2020

bryant1410 commented Feb 11, 2020 •

edited

Loading

matt-gardner left a comment •

edited

Loading