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

feat: dask expr cast #821

Merged
merged 13 commits into from
Aug 25, 2024

Conversation

aidoskanapyanov
Copy link
Contributor

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.

Implemented without preserving a dtype backend for now. Need to dig more into that :P

@github-actions github-actions bot added enhancement New feature or request internal labels Aug 20, 2024
@aidoskanapyanov aidoskanapyanov changed the title feat: dask expr cast feat: dask expr cast Aug 20, 2024
Copy link
Member

@FBruzzesi FBruzzesi left a comment

Choose a reason for hiding this comment

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

I love this! Thanks for the effort! πŸ™ŒπŸΌ
I left a few nitpicks here and there πŸ˜‡

Regarding the failing test due to missing coverage, branches can't be all reach in the same test run πŸ₯²

narwhals/_dask/utils.py Outdated Show resolved Hide resolved
tests/selectors_test.py Outdated Show resolved Hide resolved
narwhals/_dask/expr.py Outdated Show resolved Hide resolved
@aidoskanapyanov
Copy link
Contributor Author

aidoskanapyanov commented Aug 21, 2024

Thanks for the review! :)

Some CI failures:

  • Coverage is dropping for some reason for pytest-38 (3.8, windows-latest)
  • Also, modin constructor is not failing, but marked as failing in pytest

@FBruzzesi
Copy link
Member

FBruzzesi commented Aug 22, 2024

  • Coverage is dropping for some reason for pytest-38 (3.8, windows-latest)

I will take a look and see if I can spot anything

  • Also, modin constructor is not failing, but marked as failing in pytest

That's typically good news even though... why is that happening now πŸ˜‚?

Edit: Feel free to remove such block and check against CI. Locally I was able to get all green dots, but I can't replicate on windows.

narwhals/_dask/utils.py Outdated Show resolved Hide resolved
@aidoskanapyanov
Copy link
Contributor Author

I wonder why windows and linux pytest coverage CI show different results. For some reason windows run is flagging lots of lines as uncovered, whereas linux is flagging them as green.

For example, on my linux machine coverage report is saying all good for these lines:
image

while the windows CI run is flagging lines 84-121:

2024-08-22T18:19:06.5566828Z narwhals\_dask\utils.py                             72     60     46      0    10%   18-45, 51-75, 79-80, 84-121

@aidoskanapyanov
Copy link
Contributor Author

I wonder why windows and linux pytest coverage CI show different results. For some reason windows run is flagging lots of lines as uncovered, whereas linux is flagging them as green.

Yeah, nevermind that, I figure CI coverage reports are a bit more involved than I understand rn. Gotta dig into it first.

@FBruzzesi
Copy link
Member

Hey there!

I think the problem with coverage for python 3.8 is that dask is entirely skipped (min python version is 3.9), therefore as we increase total number of functions, the coverage will gradually decrease.

I think you just unluckily hit the threshold number to push the total below 90%.

As all other workflows are passing, I would not worry too much about it. I think we will need to be smarter about it or come with another threshold value.

Copy link
Member

@FBruzzesi FBruzzesi left a comment

Choose a reason for hiding this comment

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

We adjusted the threshold to 85%, now some other unrelated CI error appeared πŸ˜‚

uv pip install --pre --extra-index-url https://pypi.anaconda.org/scientific-python-nightly-wheels/simple pandas --system
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.12.5/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.12.5/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.5/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.5/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.5/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.12.5/x64/lib
error: HTTP status server error (503 Service Unavailable) for url (https://pypi.anaconda.org/scientific-python-nightly-wheels/simple/pandas/)
Error: Process completed with exit code 2.

@aidoskanapyanov
Copy link
Contributor Author

aidoskanapyanov commented Aug 25, 2024

Thanks @FBruzzesi !
Yeah, server error on the anaconda side by the looks of it. I've rerun the CI manually, it's passing now πŸ˜„

@aidoskanapyanov aidoskanapyanov merged commit 706b7c8 into narwhals-dev:main Aug 25, 2024
21 checks passed
@aidoskanapyanov aidoskanapyanov deleted the feat_dask_expr_cast branch August 25, 2024 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants