Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Order variables by closeness to executing statement in pure_eval #807

Merged
merged 2 commits into from Sep 2, 2020

Conversation

alexmojaki
Copy link
Contributor

Part of #805

The idea here is to prioritise values which appear closer to the executing statement. The dict of values is ordered so that trimming removes lower priority values later on.

However this doesn't work in python 3.5 because the ordered dict is actually converted to a regular dict which loses the order:

# Create temporary copy here to avoid calling too much code that
# might mutate our dictionary while we're still iterating over it.
obj = dict(iteritems(obj))

It seems that my options are:

  1. Remove the above line and fix the errors that arise.
  2. Keep the dict ordered when making the copy.
  3. Preemptively trim in the pure_eval integration so order no longer matters.

What do you think @untitaker ?

@vmarkovtsev
Copy link

Note: this will automatically include the function arguments, because, for all the lines except the lowest in the call stack, they lie on an executing statement 👍 I am super excited and eager to try this 🎉

Copy link
Member

@untitaker untitaker left a comment

Choose a reason for hiding this comment

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

tests fail on tox -e py3.5-pure_eval, but this looks good

"u",
"y",
]
if sys.version_info[:2] == (3, 5):
Copy link
Member

Choose a reason for hiding this comment

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

Instead of this conditional can't we do assert set(frame_vars.keys()) == set(expected_keys)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you misunderstood. frame_vars.keys() is set-like and so assert frame_vars.keys() == set(expected_keys) would work in all versions. assert list(frame_vars.keys()) == expected_keys is a stronger assertion to make extra sure (when possible) that values are prioritised as expected. The tests are fine as they are, the reason for the failure in 3.5 is that the implementation actually doesn't work and the values are actually wrong.

The problem is that I return OrderedDict above so that the trimming in serialize will keep the first 10 items of the dict, but just before the trimming the OrderedDict gets converted to a dict (see the PR description) which breaks the order in 3.5 and thus the implementation (the wrong values get trimmed). I'm asking if I should just trim the values within the pure_eval integration and forget the OrderedDict, or try to preserve order in serialize.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be clear both options should be equivalent right now, but if serialize changes its algorithm and starts trimming things more cleverly in the future, it will be better if it has the full dict of variables in order of priority rather than a dict that was eagerly/preemptively trimmed by pure_eval.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right, I recommend going for option 3 but avoiding hardcoding the number 10 (instead importing the var). I don't think there's a safe alternative to dict outside of pure_eval context, so I'd like to avoid changes to serializer.

Copy link
Member

Choose a reason for hiding this comment

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

but if serialize changes its algorithm and starts trimming things more cleverly in the future

I doubt this will happen in serialization, let's assume pure_eval always knows better (seems likely given the amount of extra analysis it does)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@untitaker untitaker merged commit 6541785 into getsentry:master Sep 2, 2020
@untitaker
Copy link
Member

Excellent, thank you!

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.

None yet

3 participants