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

SNOW-1672691: Remove snowflake/snowpark/modin/pandas folder #2345

Merged
merged 8 commits into from
Sep 26, 2024

Conversation

sfc-gh-joshi
Copy link
Contributor

@sfc-gh-joshi sfc-gh-joshi commented Sep 23, 2024

  1. Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes SNOW-1672691

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
      • If this test skips Local Testing mode, I'm requesting review from @snowflakedb/local-testing
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am adding new credentials
    • I am adding a new dependency
    • If this is a new feature/behavior, I'm adding the Local Testing parity changes.
  3. Please describe how your code solves the related issue.

This PR removes the snowflake/snowpark/modin/pandas folder. Remaining docstring paths have been modified appropriately, and necessary initialization code has moved to snowflake/snowpark/modin/plugin/init.py.

Previously, we exposed methods from general_overrides and io_overrides as direct exports from the snowflake.snowpark.modin.pandas that were then mirrored into the modin.pandas namespace. Now, we directly call modin.pandas.api.extensions.register_pd_accessor to populate modin.pandas.

@sfc-gh-joshi sfc-gh-joshi added NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md NO-PANDAS-CHANGEDOC-UPDATES This PR does not update Snowpark pandas docs and removed NO-PANDAS-CHANGEDOC-UPDATES This PR does not update Snowpark pandas docs labels Sep 23, 2024
@sfc-gh-joshi sfc-gh-joshi marked this pull request as ready for review September 24, 2024 19:02
@sfc-gh-joshi sfc-gh-joshi requested review from a team as code owners September 24, 2024 19:02
@sfc-gh-joshi
Copy link
Contributor Author

(triggered Snowpark Python review because of changes to setup.py, no other changes on the Python side were made)

# base overrides occur before subclass overrides in case subclasses override a base method
import snowflake.snowpark.modin.plugin.extensions.base_extensions # isort: skip # noqa: E402,F401
import snowflake.snowpark.modin.plugin.extensions.base_overrides # isort: skip # noqa: E402,F401
import snowflake.snowpark.modin.plugin.extensions.dataframe_extensions # isort: skip # noqa: E402,F401
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sfc-gh-joshi where did groupby go? i don't see groupby here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need to explicitly import groupby/resample/window/indexing overrides because they don't trigger any modin.api.extensions.register_*_accessor calls. Instead, we override the call to DataFrame.groupby(), DataFrame.rolling, and similar methods to just return a Snowpark pandas DataFrameGroupBy/Window object defined in groupby_overrides.py/window_overrides.py.

@sfc-gh-joshi sfc-gh-joshi merged commit 5390eb5 into main Sep 26, 2024
35 checks passed
@sfc-gh-joshi sfc-gh-joshi deleted the joshi/rip-pandas branch September 26, 2024 22:05
@github-actions github-actions bot locked and limited conversation to collaborators Sep 26, 2024
@sfc-gh-mvashishtha
Copy link
Contributor

Good riddance to snowflake/snowpark/modin/pandas !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md snowpark-pandas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants