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

(refactor): Enable new engines with custom dispatching and other constructs #1666

Merged
merged 6 commits into from
Oct 11, 2022
Merged

(refactor): Enable new engines with custom dispatching and other constructs #1666

merged 6 commits into from
Oct 11, 2022

Conversation

jaidisido
Copy link
Contributor

@jaidisido jaidisido commented Oct 8, 2022 •
edited

Feature or Bugfix

  • Refactoring

Detail

The current codebase cannot easily support multiple engines. The below changes remediate that:

  • Move distributed methods out of non-distributed modules
  • Add a custom dispatcher and use it to register methods based on the execution engine and memory format value
  • Add classes to manage the state of the execution engine and memory format
  • Refactor the structure of distributed modules to support multiple engines

Code flow:

  1. Engine class is initialised at import
  2. It detects the installed engine (python vs ray), initialises the engine and registers the dispatched methods based on the value of the engine and memory format (using this custom dispatcher)

The good:

  • Clear separation of concerns: Distributed methods live outside non-distributed code, eliminating ugly if conditionals, allowing both to scale independently and making them easier to maintain in the future
  • Better dispatching: Adding a new engine/memory format is as simple as creating a new directory with its methods and registering them with the custom dispatcher based on the value of the engine or memory format
  • Custom engine/memory format classes: Give more flexibility than config when it comes to interacting with the engine and managing its state (initialising, registering, get/setting...)

The bad:

  • Managing state: Adding a custom dispatcher means that we must maintain its state. For instance, one capability currently missing in this PR is unregistering methods when a user sets a different engine (e.g. moving from ray to dask at execution time).
  • Detecting the engine: Conditionals are simpler/easier when it comes to detecting an engine. With a custom dispatcher, the registration and dispatching process is more opaque/convoluted. For instance, there is a higher risk of not realising that we are using a given engine vs another

The ugly:

  • Unused arguments: Each method registered with the dispatcher must accept the union of both non-distributed and distributed arguments, even though some would be unused. As the list of supported engines grows, so does the number of unused arguments. It also means that we must maintain the same list of arguments across the different versions of the method

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jaidisido
Major refactoring:
7e5b8e3 
- Move distributed methods out of non-distributed modules
- Refactor dispatching
- Refactor structure of distributed modules
- Add classes for execution engine and memory format
@jaidisido jaidisido self-assigned this Oct 8, 2022
@jaidisido jaidisido marked this pull request as draft October 8, 2022 10:47
@malachi-constant
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: GitHubDistributedCodeBuild6-jWcl5DLmvupS
  • Commit ID: 7e5b8e3
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@malachi-constant
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: GitHubLoadTests5656BB24-s6u9F3qN9oFy
  • Commit ID: 7e5b8e3
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@malachi-constant
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: GitHubStandardCodeBuild8C06-llutOAimTATs
  • Commit ID: 7e5b8e3
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@jaidisido jaidisido marked this pull request as ready for review October 8, 2022 16:36
@jaidisido jaidisido added the major release Will be addressed in the next major release label Oct 8, 2022
@jaidisido jaidisido added this to the 3.0.0 milestone Oct 8, 2022
@jaidisido jaidisido added this to In Review in AWS SDK for pandas roadmap Oct 8, 2022
LeonLuttenberger
LeonLuttenberger approved these changes Oct 10, 2022
View reviewed changes
Copy link
Contributor

@LeonLuttenberger LeonLuttenberger left a comment

Choose a reason for hiding this comment

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

Awesome work!

awswrangler/distributed/ray/_register.py Outdated

_batch_paths.register(execution_engine, _batch_paths_distributed) # type: ignore

if memory_format.get() == MemoryFormatEnum.MODIN.value:
Copy link
Contributor

Choose a reason for hiding this comment

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

Really cool, I love how this is out of way!

Copy link
Contributor

Choose a reason for hiding this comment

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

+1. Minor Q - can memory_format.get() return an enum so you don't have to do .value on every comparison?

@malachi-constant
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: GitHubDistributedCodeBuild6-jWcl5DLmvupS
  • Commit ID: 76ac88e
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@malachi-constant
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: GitHubLoadTests5656BB24-s6u9F3qN9oFy
  • Commit ID: 76ac88e
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@malachi-constant
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: GitHubStandardCodeBuild8C06-llutOAimTATs
  • Commit ID: 76ac88e
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@malachi-constant
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: GitHubDistributedCodeBuild6-jWcl5DLmvupS
  • Commit ID: c511d30
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@malachi-constant
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: GitHubStandardCodeBuild8C06-llutOAimTATs
  • Commit ID: c511d30
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@malachi-constant
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: GitHubLoadTests5656BB24-s6u9F3qN9oFy
  • Commit ID: c511d30
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@malachi-constant
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: GitHubDistributedCodeBuild6-jWcl5DLmvupS
  • Commit ID: 9fd4093
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@malachi-constant
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: GitHubLoadTests5656BB24-s6u9F3qN9oFy
  • Commit ID: 9fd4093
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@malachi-constant
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: GitHubStandardCodeBuild8C06-llutOAimTATs
  • Commit ID: 9fd4093
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

kukushking
kukushking reviewed Oct 11, 2022
View reviewed changes
awswrangler/distributed/ray/_register.py
engine.register_func(_read_parquet_metadata_file, ray_remote(_read_parquet_metadata_file))
engine.register_func(_select_query, ray_remote(_select_query))
engine.register_func(_select_object_content, ray_remote(_select_object_content))
engine.register_func(_wait_object_batch, ray_remote(_wait_object_batch))
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about keeping the register only for registration and moving the function definitions to modin module with the others? E.g.

distributed/ray/modin/s3/...py:

_wait_object_batch_distributed = ray_remote(_wait_object_batch)
_select_object_content_distributed = ray_remote(_select_object_content)

Also it seems now we do not actually need our own ray_remote decorator that handled both distributed and non-distributed calls dynamically and we can use default @remote.

kukushking
kukushking approved these changes Oct 11, 2022
View reviewed changes
Copy link
Contributor

@kukushking kukushking left a comment

Choose a reason for hiding this comment

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

Awesome!

@malachi-constant
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: GitHubDistributedCodeBuild6-jWcl5DLmvupS
  • Commit ID: fadfdf7
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@malachi-constant
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: GitHubLoadTests5656BB24-s6u9F3qN9oFy
  • Commit ID: fadfdf7
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@malachi-constant
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: GitHubStandardCodeBuild8C06-llutOAimTATs
  • Commit ID: fadfdf7
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@malachi-constant
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: GitHubDistributedCodeBuild6-jWcl5DLmvupS
  • Commit ID: fadfdf7
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@malachi-constant
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: GitHubDistributedCodeBuild6-jWcl5DLmvupS
  • Commit ID: eb8e42d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@malachi-constant
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: GitHubLoadTests5656BB24-s6u9F3qN9oFy
  • Commit ID: eb8e42d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@malachi-constant
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: GitHubStandardCodeBuild8C06-llutOAimTATs
  • Commit ID: eb8e42d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@jaidisido jaidisido merged commit a826503 into release-3.0.0 Oct 11, 2022
@jaidisido jaidisido deleted the refactor-dispatching branch October 11, 2022 16:53
@jaidisido jaidisido moved this from In Review to Done in AWS SDK for pandas roadmap Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kukushking kukushking kukushking approved these changes

@malachi-constant malachi-constant malachi-constant approved these changes

@LeonLuttenberger LeonLuttenberger LeonLuttenberger approved these changes

@cnfait cnfait Awaiting requested review from cnfait

Assignees

@jaidisido jaidisido

Labels
major release Will be addressed in the next major release refactoring
Projects
AWS SDK for pandas roadmap
Done
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jaidisido @malachi-constant @kukushking @LeonLuttenberger