This is an archive of the discontinued LLVM Phabricator instance.

[LNT] Python 3 support: adapt to dict method returning views
ClosedPublic

Authored by thopre on Sep 20 2019, 1:34 AM.

Details

Summary

Adapt calls to dictionary's keys(), items() and values() to them
returning a view in Python 3 when necessary, undoing unneeded changes
in lnt/testing/__init__.py. Also replace calls to their iter relatives
to use iter() instead and calls to the their views relatives to use the
plains .keys(), .items() and .values(). This was produced by running
futurize's stage2 lib2to3.fixes.fix_dict with manual intervention to
clean up code and remove unnecessary list() wrapping.

Event Timeline

thopre created this revision.Sep 20 2019, 1:34 AM
examples/run_to_csv.py
10

I believe this use of next() is one of the ones that need to be replaced in the style of D67812. This file seems to be unmaintained and untested. None of the *.json files in the tests/ directory seems to match the format this script expects. I would suggest deleting this file.

15

I believe the change on this line is unnecessary.

lnt/lnttool/admin.py
32 ↗(On Diff #220972)

I believe the change on this line is unnecessary.

50 ↗(On Diff #220972)

I believe the change on this line is unnecessary. I'm not marking further instances, but we generally shouldn't form a list just to iterate.

lnt/server/db/rules/rule_update_fixed_regressions.py
78 ↗(On Diff #220972)

I think this creates a list and then creates a sorted copy. I am inclined to leave the line unchanged (and for similar instances elsewhere in this patch).

lnt/server/ui/views.py
958

Is this use of sort something that would be considered within the scope of D67809?

1018

This also seems to be used just to iterate through once.

1322

Another sorted over a fresh list.

lnt/testing/profile/profilev2impl.py
273

keys.extend(f['counters'].keys())?

318

I guess the sorted() should go in place of list() where the latter is added a few lines up.

lnt/tests/nt.py
1484

Another call to sort. Maybe use something like the following instead?

sorted(filter(lambda bench: bench.is_rerunable(),
              collated_results.values()),
       key=lambda bench: bench.name)
1489

len(collated_results)

thopre updated this revision to Diff 221278.Sep 23 2019, 3:41 AM
thopre marked 19 inline comments as done.
  • Remove all wrapping into list that are unecessary due to views being iterable
  • Improve uses of views to avoid introducing a call to list() when possible, as per Hubert
thopre added inline comments.Sep 23 2019, 3:41 AM
examples/run_to_csv.py
10

I prefer to apply the change for now and remove it or fix it in a later patch if that's ok. Strange the .next logic didn't detect this case. I've double checked the whole codebase for a call to .next() and didn't find any in python files.

lnt/lnttool/admin.py
50 ↗(On Diff #220972)

Is there no multithreading going on in LNT?

lnt/server/ui/views.py
958

I think the 2to3 fix_idioms only fires if sort is called without parameter. Presumably if the call to sort takes some parameters that might make the folding both lines into sorted too long and thus be difficult to read. I think in this specific case it would indeed be harder to read so I'm enclined to leave it like this.

1497

testsuites is only used for iterating below so I removed the list here.

lnt/testing/__init__.py
260

I'm removing the list here which I added when committing https://reviews.llvm.org/D65751

lnt/tests/nt.py
1484

As mentioned, the 2to3 sort change seems to skip this sort of folding intentionally, presumably to avoid a multiline expression. I'm therefore sticking to having filter and sort as separate steps. I did however changed to use the filter builtin as it makes the intent clearer IMHO.

lnt/util/multidict.py
19–26 ↗(On Diff #220972)

All uses of these methods already assume this returns views and have been converted to use list if necessary. Ultimately we should remove that file and rely on the pypi package IMHO.

hubert.reinterpretcast marked 2 inline comments as done.Sep 23 2019, 8:37 AM
hubert.reinterpretcast added inline comments.
examples/run_to_csv.py
10

Okay.

lnt/lnttool/admin.py
50 ↗(On Diff #220972)

config is local to the function, so I don't think it is being concurrently modified here.

lnt/server/ui/views.py
958

Okay.

lnt/tests/nt.py
1489

I'm not seeing the change. len applied to the dictionary directly works.

thopre updated this revision to Diff 221416.Sep 23 2019, 2:47 PM
thopre marked 5 inline comments as done.

Apply len to dictionary rather than dictionary values

lnt/lnttool/admin.py
50 ↗(On Diff #220972)

I meant in general, I don't know enough the code to make a good judgement call. However IIRW iterating on a views while it is being updated raises an assertion so in the worse case it should be quite obvious what is going on.

lnt/tests/nt.py
1489

Oh my bad, missed your point.

thopre edited the summary of this revision. (Show Details)Sep 23 2019, 2:53 PM
This revision is now accepted and ready to land.Sep 24 2019, 8:48 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2019, 12:33 AM