Skip to content

fix: Include all backends in get_native_namespace #2608

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.

Sign up for GitHub

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

Jump to bottom
Merged
MarcoGorelli merged 3 commits into from
May 24, 2025
Merged

fix: Include all backends in get_native_namespace #2608

MarcoGorelli merged 3 commits into from
May 24, 2025

Conversation

@FBruzzesi
Copy link
Member

@FBruzzesi FBruzzesi commented May 24, 2025

What type of PR is this? (check all applicable)

  • 💾 Refactor
  • ✨ Feature
  • 🐛 Bug Fix
  • 🔧 Optimization
  • 📝 Documentation
  • ✅ Test
  • 🐳 Other

Related issues

Checklist

  • Code follows style guide (ruff)
  • Tests added
  • Documented the changes

If you have comments or can explain your changes, please do so below

@FBruzzesi FBruzzesi added the fix label May 24, 2025
FBruzzesi
FBruzzesi commented May 24, 2025
View reviewed changes
tests/translate/get_native_namespace_test.py Outdated
assert nw.get_native_namespace(df["a"].to_native()) is pl
assert nw.get_native_namespace(df, df["a"].to_native()) is pl

def _get_expected_namespace(constructor_name: str) -> ModuleType | None: # noqa: PLR0911
Copy link
Member Author

@FBruzzesi FBruzzesi May 24, 2025

Choose a reason for hiding this comment

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

It's kind of a tautology 🤔 but I didn't want to reuse Implementation methods

MarcoGorelli
MarcoGorelli approved these changes May 24, 2025
View reviewed changes
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

🔥

@MarcoGorelli MarcoGorelli merged commit a0e6b9e into main May 24, 2025
32 checks passed
@MarcoGorelli MarcoGorelli deleted the fix/get-native-namespace branch May 24, 2025 14:05
@dangotbanned
Copy link
Member

dangotbanned commented May 24, 2025

@FBruzzesi 🥳

I think this means we can tick off another function in #2492

@dangotbanned dangotbanned mentioned this pull request May 24, 2025
Open
8 tasks
@MarcoGorelli
Copy link
Member

MarcoGorelli commented May 24, 2025

d'oh we should've run the pytest tests here

E       AssertionError: assert <module 'pyspark.sql.connect' from '/opt/hostedtoolcache/Python/3.11.12/x64/lib/python3.11/site-packages/pyspark/sql/connect/__init__.py'> is None
E        +  where <module 'pyspark.sql.connect' from '/opt/hostedtoolcache/Python/3.11.12/x64/lib/python3.11/site-packages/pyspark/sql/connect/__init__.py'> = <function get_native_namespace at 0x7f1a2d27f600>(┌────────────────────┐\n| Narwhals LazyFrame |\n|--------------------|\n|DataFrame[a: bigint]|\n└────────────────────┘)
E        +    where <function get_native_namespace at 0x7f1a2d27f600> = nw.get_native_namespace

tests/translate/get_native_namespace_test.py:66: AssertionError

@FBruzzesi
Copy link
Member Author

FBruzzesi commented May 24, 2025 •
edited
Loading

d'oh we should've run the pytest tests here

E       AssertionError: assert <module 'pyspark.sql.connect' from '/opt/hostedtoolcache/Python/3.11.12/x64/lib/python3.11/site-packages/pyspark/sql/connect/__init__.py'> is None
E        +  where <module 'pyspark.sql.connect' from '/opt/hostedtoolcache/Python/3.11.12/x64/lib/python3.11/site-packages/pyspark/sql/connect/__init__.py'> = <function get_native_namespace at 0x7f1a2d27f600>(┌────────────────────┐\n| Narwhals LazyFrame |\n|--------------------|\n|DataFrame[a: bigint]|\n└────────────────────┘)
E        +    where <function get_native_namespace at 0x7f1a2d27f600> = nw.get_native_namespace

tests/translate/get_native_namespace_test.py:66: AssertionError

How is pyspark entering the test?

narwhals/tests/translate/get_native_namespace_test.py

Lines 60 to 61 in 53d55c3

if constructor_name == "pyspark_lazy_constructor":
pytest.skip(reason="Requires special handling for spark local vs spark connect")

Nevermind - I am dumb 🥲

@FBruzzesi
Copy link
Member Author

FBruzzesi commented May 24, 2025

I added one way to solve it: 2d92f9a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MarcoGorelli MarcoGorelli MarcoGorelli approved these changes

Assignees

No one assigned

Labels

fix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: _get_native_namespace_single_obj missing a few supported libraries

4 participants

@FBruzzesi @dangotbanned @MarcoGorelli