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

feat(datasets): Improved Dependency Management for Spark-based Datasets #911

Merged
merged 30 commits into from
Nov 14, 2024
Merged

feat(datasets): Improved Dependency Management for Spark-based Datasets #911

merged 30 commits into from
Nov 14, 2024

Conversation

MinuraPunchihewa
Copy link
Contributor

@MinuraPunchihewa MinuraPunchihewa commented Oct 26, 2024 •
edited
Loading

Description

This PR a _utils sub-package to house modules with common utility functions that are used across Spark-based datasets. This avoids the need for pyspark to be installed for datasets that will run on Databricks.

Fixes #849

Development notes

The new _utils package organized the utility functions in three main modules,

  1. databricks_utils.py
  2. spark_utils.py

Additional modules can be added to this sub-package to house code that is used in multiple datasets.

These changes have been tested,

  1. Manually, by running the code locally using ManagedTableDataset and ExternalTableDataset.
  2. Via the existing unit tests.

Checklist

  • 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 relevant RELEASE.md file
  • Added tests to cover my changes
  • Received approvals from at least half of the TSC (required for adding a new, non-experimental dataset)

@MinuraPunchihewa MinuraPunchihewa changed the title Improved Dependency Management for Spark-based Datasets feat(datasets): Improved Dependency Management for Spark-based Datasets Oct 26, 2024
@MinuraPunchihewa MinuraPunchihewa marked this pull request as ready for review October 27, 2024 05:24
@MinuraPunchihewa
Copy link
Contributor Author

Hey @noklam,
I have to test this a little more thoroughly, but can you give me your opinion on the approach taken here?

@merelcht merelcht requested a review from noklam October 31, 2024 10:18
noklam
noklam reviewed Oct 31, 2024
View reviewed changes
kedro-datasets/kedro_datasets/_utils/file_utils.py Outdated
Comment on lines 1 to 18
import os


def parse_glob_pattern(pattern: str) -> str:
special = ("*", "?", "[")
clean = []
for part in pattern.split("/"):
if any(char in part for char in special):
break
clean.append(part)
return "/".join(clean)


def split_filepath(filepath: str | os.PathLike) -> tuple[str, str]:
split_ = str(filepath).split("://", 1)
if len(split_) == 2: # noqa: PLR2004
return split_[0] + "://", split_[1]
return "", split_[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK, these are used for Spark only. I do not want other datasets to use it because it's more like a workaround to handle Spark/Dtabricks magic path.

Most of the dataset should be using the util coming from kedro instead.

kedro-plugins/kedro-datasets/kedro_datasets/pandas/csv_dataset.py

Lines 17 to 19 in 1a18c5a

Version,
get_filepath_str,
get_protocol_and_path,

Copy link
Contributor Author

@MinuraPunchihewa MinuraPunchihewa Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@noklam Understood. I will move these to the spark_utils.py module.

@MinuraPunchihewa
moved the file based utility funcs to databricks_utils
8f7842f 
Signed-off-by: Minura Punchihewa <[email protected]>
@MinuraPunchihewa
fixed the imports of the file based utility funcs
6e50319 
Signed-off-by: Minura Punchihewa <[email protected]>
@MinuraPunchihewa
fixed lint issues
63834a9 
Signed-off-by: Minura Punchihewa <[email protected]>
@MinuraPunchihewa
fixed the use of _get_spark() in tests
6df7741 
Signed-off-by: Minura Punchihewa <[email protected]>
@MinuraPunchihewa
fixed uses of databricks utils in tests
80f9fb5 
Signed-off-by: Minura Punchihewa <[email protected]>
@MinuraPunchihewa
fixed more tests
03da501 
Signed-off-by: Minura Punchihewa <[email protected]>
@MinuraPunchihewa
fixed more lint issues
0534907 
Signed-off-by: Minura Punchihewa <[email protected]>
@MinuraPunchihewa
fixed more tests
65d1c30 
Signed-off-by: Minura Punchihewa <[email protected]>
@MinuraPunchihewa
fixed more tests
f7f0b5e 
Signed-off-by: Minura Punchihewa <[email protected]>
@MinuraPunchihewa
Copy link
Contributor Author

Hey @noklam,
I've now had the opportunity to test out these changes and they seem to work fine. I've tested both ManagedTableDataset and ExternalTableDataset with the reduced dependencies without any issues.

noklam
noklam reviewed Oct 31, 2024
View reviewed changes
Copy link
Contributor

@noklam noklam left a comment

Choose a reason for hiding this comment

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

Some more comments I left but I only notice it wasn't sent properly 😅

kedro-datasets/kedro_datasets/_utils/databricks_utils.py Outdated
return sorted(matched)


def get_dbutils(spark: SparkSession) -> Any:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this always a SparkSession? I think it can be a DatabricksSession

kedro-datasets/kedro_datasets/_utils/spark_utils.py Outdated
from pyspark.sql import SparkSession


def get_spark() -> Any:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def get_spark() -> Any:
def get_spark() -> Any:

I think this should be SparkSession and DatabricksSession? You should also wrap it inside TYPE_CHECKING to avoid import error.

@MinuraPunchihewa
Copy link
Contributor Author

Some more comments I left but I only notice it wasn't sent properly 😅

Haha no problem. I've made the suggested improvements to the type hints, including a couple more involving DBUtils.

@noklam noklam self-requested a review November 7, 2024 12:53
noklam
noklam approved these changes Nov 7, 2024
View reviewed changes
Copy link
Contributor

@noklam noklam 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 addressing all the comments and your contribution on the databricks dataset, great stuff!

@DimedS DimedS added the Community Issue/PR opened by the open-source community label Nov 8, 2024
merelcht
merelcht approved these changes Nov 13, 2024
View reviewed changes
Copy link
Member

@merelcht merelcht 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 this contribution @MinuraPunchihewa ! ⭐ Can you update the release notes and add your change + your name to contributors?

@MinuraPunchihewa
Copy link
Contributor Author

Thanks for this contribution @MinuraPunchihewa ! ⭐ Can you update the release notes and add your change + your name to contributors?

Thanks, @merelcht. I've just updated the release notes.

@merelcht
Reorder release notes
0f65bdb 
Signed-off-by: Merel Theisen <[email protected]>
@merelcht merelcht enabled auto-merge (squash) November 14, 2024 08:53
@merelcht merelcht merged commit 07aef5a into kedro-org:main Nov 14, 2024
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merelcht merelcht merelcht approved these changes

@noklam noklam noklam approved these changes

Assignees
No one assigned
Labels
Community Issue/PR opened by the open-source community
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

kedro-datasets: Improve dependency management for spark Datasets
4 participants
@MinuraPunchihewa @noklam @merelcht @DimedS