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

Start with dask-expr doc build #10879

Merged
merged 3 commits into from Feb 1, 2024
Merged

Start with dask-expr doc build #10879

merged 3 commits into from Feb 1, 2024

Conversation

phofl
Copy link
Collaborator

@phofl phofl commented Jan 31, 2024

  • Closes #xxxx
  • Tests added / passed
  • Passes pre-commit run --all-files

This isn't ready to be merged, there are a couple of weird failures in the accessor docs

@phofl phofl marked this pull request as draft January 31, 2024 14:14
Copy link
Contributor

github-actions bot commented Jan 31, 2024

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

     15 files  ±0       15 suites  ±0   3h 10m 33s ⏱️ + 2m 43s
 12 983 tests ±0   12 053 ✅ ±0     929 💤 ±0  1 ❌ ±0 
160 174 runs  ±0  143 595 ✅  - 5  16 576 💤 +3  3 ❌ +2 

For more details on these failures, see this check.

Results for commit 1c6e54e. ± Comparison against base commit 1b711be.

♻️ This comment has been updated with latest results.

@phofl phofl marked this pull request as ready for review January 31, 2024 15:37
@phofl
Copy link
Collaborator Author

phofl commented Jan 31, 2024

I've had to define a few accessors explicitly, since sphinx confused the DataFrame classes from Dask-expr and dask/dask without them. This is a known issue see sphinx-doc/sphinx#4961

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks @phofl -- overall this LGTM. I left a few comments, the main one I care about is the new API page being top-level in the DataFrame toc. All the other comments are just non-blocking suggestions.

dask/dataframe/accessor.py Outdated Show resolved Hide resolved
Comment on lines +1 to +2
Dask DataFrame API with Logical Query Planning
==============================================
Copy link
Member

Choose a reason for hiding this comment

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

It might be nice to add a few sentences here about the query planning effort and how to enable it. I don't mean this as a blocking comment -- we can always follow-up later, or not at all

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes I intend to add content as we go before the new release is out

docs/source/dask-expr-api.rst Outdated Show resolved Hide resolved
@@ -12,6 +12,7 @@ Dask DataFrame
Internal Design <dataframe-design.rst>
Best Practices <dataframe-best-practices.rst>
API <dataframe-api.rst>
API Query Planning <dask-expr-api.rst>
Copy link
Member

Choose a reason for hiding this comment

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

My sense is this is too top-level at the moment. I'd recommend moving this into the "Additional Information" dropdown for now unless there's a specific reason it needs to be top-level.

If it does need to be top-level, then we'll probably want more information about the query planning effort more generally that answers questions like "What is it?", "Why should I care?", "How do I enable it?", etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I'll move it down, but we ideally want to add the deprecation message soonish, would move it back up then, but we will hopefully have more content by then

@phofl phofl merged commit 08341db into dask:main Feb 1, 2024
24 of 28 checks passed
@phofl phofl deleted the dask-expr-docs branch February 1, 2024 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants