Dict memoization sorting before normalizing keys is unsafe #3157
Conversation
MundiaNderi
changed the title
|
@benclifford I've worked on the memoization issue but I still have some conflicts with the max-length-files . How do I proceed ? |
|
You've got a lot of other random stuff in your PR that isn't related to issue #2140 - maybe you typed For example, lots of test output in runinfo/ is there in your PR... Have a look at this URL to see it all https://github.com/Parsl/parsl/pull/3157/files You can remove the files you don't want with It also looks like you have a few other changes with whitespace and new lines that aren't relevant to your PR. |
|
@benclifford You're right. I used the git add command. Let me remove the files I don't want |
| # keys = sorted(denormalized_dict) Line that sirosen commented on | ||
| # Proposed solution was to normalize the keys and then sort them | ||
| keymap = {id_for_memo(k): k for k in denormalized_dict} | ||
| normed_keys = sorted(keymap.values()) |
There was a problem hiding this comment.
ok, I think this is right thing to be doing - but I don't think your test case that you added really tests that something was broken before, and is now fixed by this change.
I will try to add some comments on the test case if I can figure out a stronger test case.
There was a problem hiding this comment.
Thank you , I've also realised that and was trying to think though it. Where I fell short was I corrected the implementation first, then created the test case later. I should have done vice versa.
|
there are still a lot of changes in this pull request that are not directly related to the dictionary sorting issue: look in the "Files Changed" tab on this pull request in the Github web interface, and you will see a lot of unrelated changes still. |
|
Okay @benclifford let me work on resolving this. |
|
@benclifford I've cleaned up the files less one file which got messed up during rebase. How would you advise me to proceed ? |
Which file? What do you think it should look like? |
|
This file: parsl/dataflow/dflow.py. git rm parsl/dataflow/dflow.py deleted the file locally instead of from the commit. Ideally it should not be present within this PR but rebasing and merging . |
|
It looks like the change to dflow.py is a straightforward line break which you can fix by joining the lines again and making a new commit with that fix. |
|
Thank you @benclifford I've fixed the file. |
Description
Stability of sorting and transforming keys into a format suitable for sorting.
Changed Behaviour
Prior sorting was done before normalising the keys. This implementation normalises the keys and then sorts them. Added a test case that fails should this not be the case.
Fixes
Fixes #2140
Type of change