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.
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
SNOW-1119855: Remove modin/pandas/base.py (2/2) #2167
SNOW-1119855: Remove modin/pandas/base.py (2/2) #2167
Changes from all commits
9f16ced
340d506
899d90d
3d9c5c3
72f76c2
87d75fa
880aa05
3be10dc
2b58764
3be5616
0939b52
01f40d9
c42b4d0
0956d1c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems we will have to overwrite the class definition regardless, otherwise the telemetry will not work. Or is modin providing any telemetry infra for use? i recall @sfc-gh-azhan and @sfc-gh-nkrishna was doing something for telemetry. I don't think we need to block on this pr for this, let's follow up offline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modin has its own telemetry mechanisms, but there's not really a good way for us to hook into it to send data back to Snowflake.
As far as I can tell, the
TelemetryMeta
class basically wraps every method/property of the class in a flavor ofsnowpark_pandas_telemetry_decorator
, so once we fully use the modin frontend, we could monkeypatch those methods at runtime to have this behavior.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i recall that you mentioned that we need to overwrite the class definition for Series, so that it can take snowpandas index as an input, if we also put metaclass=TelemetryMeta when overwrite the class definition, would it work?
if we do it as monkey patch later, how would it look like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what I referred to was overriding the
__init__
method ofSeries
, not the actual class declaration itself. If we were to override the class declaration, it wouldn't be much different from what we're currently doing.My thinking is that when we remove Series/DF, we do something like this:
which would overwrite the existing method with a telemetry-enabled version.