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.

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

Reorganise the documentation structure for Kedro / Databricks integration #2442

Merged
merged 26 commits into from
Mar 30, 2023
Merged

Reorganise the documentation structure for Kedro / Databricks integration #2442

merged 26 commits into from
Mar 30, 2023

Conversation

jmholzer
Copy link
Contributor

@jmholzer jmholzer commented Mar 20, 2023 •
edited
Loading

Description

Resolves #2436

Development notes

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the RELEASE.md file
  • Added tests to cover my changes

@jmholzer jmholzer mentioned this pull request Mar 20, 2023
Merged
5 tasks
@jmholzer jmholzer requested a review from stichbury March 22, 2023 16:38
@jmholzer jmholzer marked this pull request as ready for review March 22, 2023 16:39
@jmholzer jmholzer removed the request for review from yetudada March 22, 2023 16:39
@jmholzer
Copy link
Contributor Author

Docs rendered at https://kedro--2442.org.readthedocs.build/en/2442/.

stichbury
stichbury reviewed Mar 23, 2023
View reviewed changes
docs/source/deployment/deployment_guide.md
@@ -15,7 +15,6 @@ We also provide information to help you deploy to the following:
* to [Prefect](prefect.md)
* to [Kubeflow Workflows](kubeflow.md)
* to [AWS Batch](aws_batch.md)
* to [Databricks](databricks.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should still retain a link to the databricks deployment guide, but point through to its new location, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, but since the old 'deployment' guide is now a development guide, we don't have one to link to. I'll add a link to the deployment guide here in the PR that adds it.

stichbury
stichbury approved these changes Mar 23, 2023
View reviewed changes
Copy link
Contributor

@stichbury stichbury left a comment

Choose a reason for hiding this comment

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

I'm happy with this layout. There are copy changes I want to make across the board, but that doesn't affect this PR, which is more about organisation, so I'm happy to approve it. Thank you 🙏

@astrojuanlu
Copy link
Member

linkcheck failures are solved by #2459

astrojuanlu
astrojuanlu reviewed Mar 24, 2023
View reviewed changes
Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

Thanks for the patience @jmholzer ! Added some comments

docs/build-docs.sh Outdated Show resolved Hide resolved
docs/source/index.rst Outdated Show resolved Hide resolved
docs/source/workflow_integration/databricks.rst Outdated Show resolved Hide resolved
docs/source/workflow_integration/visualisation.md Outdated Show resolved Hide resolved
@jmholzer jmholzer requested a review from astrojuanlu March 29, 2023 13:23
astrojuanlu
astrojuanlu approved these changes Mar 29, 2023
View reviewed changes
Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

Approved! Just left two questions

docs/source/integrations/databricks.rst Outdated Show resolved Hide resolved
docs/source/integrations/pyspark.rst
@@ -0,0 +1,9 @@

Copy link
Member

Choose a reason for hiding this comment

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

Do we need a table of contents here? In other words, could integrations/pyspark_integration.md become this page, hence having only one?

(I suppose if we want to grow this in the future, it might be a good idea to keep it just to avoid creating one redirect, but wanted to ask anyway)

Copy link
Contributor

Choose a reason for hiding this comment

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

We probably don't need it and could instead just re-title pyspark_integration.md to have a header that corresponds with the one for databricks (That is retitle to "Pyspark integration")

@jmholzer jmholzer force-pushed the docs/reorganise-databricks-structure branch from 01bfdfd to 8ff6da7 Compare March 29, 2023 22:36
@jmholzer jmholzer requested a review from idanov as a code owner March 29, 2023 22:36
@jmholzer jmholzer removed the request for review from idanov March 29, 2023 22:37
@jmholzer jmholzer force-pushed the docs/reorganise-databricks-structure branch 3 times, most recently from 402c03f to 39d1426 Compare March 29, 2023 22:54
jmholzer added 8 commits March 29, 2023 23:56
jmholzer and others added 16 commits March 29, 2023 23:56
@jmholzer
Remove spurious max depth to test if docs build
df6268b 
Signed-off-by: Jannic Holzer <[email protected]>
@jmholzer
Refactor index.rst to try to avoid build failing
bc91583 
Signed-off-by: Jannic Holzer <[email protected]>
@jmholzer
Modify call to sphinx-build to test if RTD will work
cdbb915 
Signed-off-by: Jannic Holzer <[email protected]>
@stichbury @jmholzer
Revise index.rst
dddfd86 
Signed-off-by: Jo Stichbury <[email protected]>
@stichbury @jmholzer
Lint and resolve
0f0f35f 
Signed-off-by: Jo Stichbury <[email protected]>
Signed-off-by: Jannic Holzer <[email protected]>
@jmholzer
Change title to include mention of Notebooks
cc49f43 
Signed-off-by: Jannic Holzer <[email protected]>
@jmholzer
Remove verbosity from viz on Databricks intro
27dbeda 
Signed-off-by: Jannic Holzer <[email protected]>
@jmholzer
Revert command modifcation
29d8371 
Signed-off-by: Jannic Holzer <[email protected]>
@jmholzer
Rename databricks visualisation docs
ba87618 
Signed-off-by: Jannic Holzer <[email protected]>
@jmholzer @stichbury
Add a line between copy and code snippets for rendering
3ee4717 
Co-authored-by: Jo Stichbury <[email protected]>
Signed-off-by: Jannic Holzer <[email protected]>
@jmholzer @stichbury
Remove gerund
8b5c770 
Co-authored-by: Jo Stichbury <[email protected]>
Signed-off-by: Jannic Holzer <[email protected]>
@jmholzer @stichbury
Remove spurious 'i.e.'
254775b 
Co-authored-by: Jo Stichbury <[email protected]>
Signed-off-by: Jannic Holzer <[email protected]>
@jmholzer
Rename workflow_integration to integrations
ffe161d 
Signed-off-by: Jannic Holzer <[email protected]>
@jmholzer
Rename index entry to 'Integrations'
f4533f8 
Signed-off-by: Jannic Holzer <[email protected]>
@jmholzer
Convert databricks.rst to MyST format
9f79a67 
Signed-off-by: Jannic Holzer <[email protected]>
@jmholzer
Rename databricks.rst to databricks.md
d3c727c 
Signed-off-by: Jannic Holzer <[email protected]>
@jmholzer jmholzer force-pushed the docs/reorganise-databricks-structure branch from 39d1426 to d3c727c Compare March 29, 2023 22:57
@jmholzer jmholzer force-pushed the docs/reorganise-databricks-structure branch from 37ee988 to 199a0c3 Compare March 29, 2023 23:41
@jmholzer jmholzer merged commit c2968ba into main Mar 30, 2023
@jmholzer jmholzer deleted the docs/reorganise-databricks-structure branch March 30, 2023 09:35
astrojuanlu
astrojuanlu reviewed Mar 30, 2023
View reviewed changes
docs/source/index.rst

faq/faq
faq/architecture_overview
Copy link
Member

Choose a reason for hiding this comment

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

Woops, somehow these were accidentally brought back, they were deleted in #2373. Will fix in #2459

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for noticing, unsure how this happened though we had a DCO problem which led to a huge rebase conflict on index.rst for a reason I couldn't understand, these must have crept back in then.

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

@stichbury stichbury stichbury approved these changes

@astrojuanlu astrojuanlu astrojuanlu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reorganise the documentation structure for Kedro / Databricks integration
3 participants
@jmholzer @astrojuanlu @stichbury