FIX-#4478: Support level param in query and eval. #4529
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: mvashishtha <[email protected]>
Signed-off-by: mvashishtha <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #4529 +/- ##
==========================================
+ Coverage 86.36% 88.17% +1.81%
==========================================
Files 228 229 +1
Lines 18413 18726 +313
==========================================
+ Hits 15902 16512 +610
+ Misses 2511 2214 -297
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
|
naren-ponder
reviewed
Jun 6, 2022
There was a problem hiding this comment.
Great work on this @mvashishtha. Just have a few questions, but overall looks good to me.
| # either. | ||
| level += 1 | ||
|
|
||
| frame = sys._getframe(level + 1) |
There was a problem hiding this comment.
Why are we bumping level by one again here?
| level += 1 | ||
|
|
||
| frame = sys._getframe(level + 1) | ||
| f_locals = frame.f_locals |
There was a problem hiding this comment.
Is there a reason you removed the try finally statements?
There was a problem hiding this comment.
Yes, please retain the try..finally, it's here to protect against creating a reference cycle if any exception fires during getting the variables. Maybe we should extend this finally until for name in local_names loop is over and remove f_locals and f_globals as well for the same refcycle reason...
vnlitvinov
reviewed
Jun 7, 2022
| level += 1 | ||
|
|
||
| frame = sys._getframe(level + 1) | ||
| f_locals = frame.f_locals |
There was a problem hiding this comment.
Yes, please retain the try..finally, it's here to protect against creating a reference cycle if any exception fires during getting the variables. Maybe we should extend this finally until for name in local_names loop is over and remove f_locals and f_globals as well for the same refcycle reason...
Comment on lines
+821
to
+826
| # Bump level up one because this function is wrapped in the Modin | ||
| # logging function wrapper. The alternative is for the wrapper to | ||
| # give the Modin functions that deal with `level` the special | ||
| # treatment of bumping `level` up by one, but that's not so nice | ||
| # either. | ||
| level += 1 |
There was a problem hiding this comment.
I wonder if we should just discard any modin-owned frames (or at least logging-owned) instead of calculating this offset manually
| self._validate_eval_query(expr, **kwargs) | ||
| inplace = validate_bool_kwarg(inplace, "inplace") | ||
| self._update_var_dicts_in_kwargs(expr, kwargs) | ||
| self._update_var_dicts_in_kwargs(expr, level + 1, kwargs) |
There was a problem hiding this comment.
you had already increased level above, why again +1?
| self._validate_eval_query(expr, **kwargs) | ||
| inplace = validate_bool_kwarg(inplace, "inplace") | ||
| self._update_var_dicts_in_kwargs(expr, kwargs) | ||
| self._update_var_dicts_in_kwargs(expr, level + 1, kwargs) | ||
| # Make sure to not pass level to query compiler |
There was a problem hiding this comment.
I don't think this comment is needed
| new_query_compiler = self._query_compiler.eval(expr, **kwargs) | ||
| return_type = type( | ||
| pandas.DataFrame(columns=self.columns) | ||
| .astype(self.dtypes) | ||
| .eval(expr, **kwargs) | ||
| .eval(expr, level=level + 1, **kwargs) |
Comment on lines
+1723
to
+1734
| # Remove `level` from the kwargs if it's already there. "level" doesn't | ||
| # make sense within the remote execution context, where the remote | ||
| # functions have a stack which is different from the stack on the | ||
| # driver node that ends in the modin eval call. | ||
| level = kwargs.pop("level", 0) | ||
|
|
||
| # Bump level up one because this function is wrapped in the Modin | ||
| # logging function wrapper. The alternative is for the wrapper to | ||
| # give the Modin functions that deal with `level` the special | ||
| # treatment of bumping `level` up by one, but that's not so nice | ||
| # eitiher. | ||
| level += 1 |
There was a problem hiding this comment.
given I see exact same wall of comments three times already, maybe we should turn this into a helper function like
def _extract_eval_level(kwargs):
"""
Extract `level` from eval or query `kwargs`.
Remove `level` from the kwargs if it's already there. "level" doesn't
make sense within the remote execution context, where the remote
functions have a stack which is different from the stack on the
driver node that ends in the modin eval call.
Bump level up one because eval/query are wrapped in the Modin
logging function wrapper.
"""
level = kwargs.pop("level", 0) + 1
return level, kwargsand then use it here like
def query(self, expr, inplace=False, **kwargs):
# docstring
level, kwargs = _extract_eval_level(kwargs)
# do the rest of the stuff
Comment on lines
+308
to
+309
| df_method_with_local_var(modin_df, method, expr, level=level), | ||
| df_method_with_local_var(pandas_df, method, expr, level=level), |
There was a problem hiding this comment.
I think we should also test the case of omitting the level argument (to ensure we and pandas stay on the same page about default value of level).
| @@ -795,25 +795,40 @@ def equals(self, other): # noqa: PR01, RT01, D200 | |||
| and self.eq(other).all().all() | |||
| ) | |||
|
|
|||
| def _update_var_dicts_in_kwargs(self, expr, kwargs): | |||
| def _update_var_dicts_in_kwargs(self, expr, level: int, kwargs): | |||
There was a problem hiding this comment.
nit: might as well add type annotations for expr and kwargs here.
| """ | ||
| Copy variables with "@" prefix in `local_dict` and `global_dict` keys of kwargs. | ||
|
|
||
| This function exists because we neeed to infer local and variables |
There was a problem hiding this comment.
should this say local and *global* variables here? it feels like it's missing a word.
RehanSD
requested changes
Jun 8, 2022
| infer them on their own. The remote functions take place in a different | ||
| thread with their own stacks that normally do not match the stack that | ||
| leads to the Modin eval or query call. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| expr : str | ||
| The expression string to search variables with "@" prefix. |
There was a problem hiding this comment.
Suggested change
| The expression string to search variables with "@" prefix. | |
| The expression string to search for variables with "@" prefix. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: mvashishtha [email protected]
What do these changes do?
Support level param in query and eval.
flake8 modin/ asv_bench/benchmarks scripts/doc_checker.pyblack --check modin/ asv_bench/benchmarks scripts/doc_checker.pygit commit -sdocs/development/architecture.rstis up-to-date