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

[BUGFIX] Be able to use spark execution engine with spark reuse flag #3541

Merged
merged 7 commits into from
Nov 2, 2021

Conversation

fep2
Copy link
Contributor

@fep2 fep2 commented Oct 13, 2021

Changes proposed in this pull request:

  • adding force_reuse_spark_context to Execution Engine Schema

  • My code follows the Great Expectations style guide

  • I have performed a self-review of my own code

  • I have added unit tests where applicable and made sure that new and existing tests are passing.

  • I have run any local integration tests and made sure that nothing is broken.

Thank you for submitting!

@netlify
Copy link

netlify bot commented Oct 13, 2021

👷 Deploy request for niobium-lead-7998 pending review.
Visit the deploys page to approve it

🔨 Explore the source changes: afc1e1d

@talagluck talagluck added community devrel This item is being addressed by the Developer Relations Team labels Oct 18, 2021
@talagluck
Copy link
Contributor

Thanks so much for opening this PR, @fep2! We will review and be in touch in the next couple of days. In the meantime, it appears that some tests are failing. Would you mind taking a look and seeing if you can resolve? Please let me know if you need any help with that.

@talagluck
Copy link
Contributor

Hi @fep2 - just wanted to check in, would you be up for seeing which tests are failing and fixing them? Thanks!

@fep2
Copy link
Contributor Author

fep2 commented Oct 26, 2021

@talagluck I found the issue for why it breaks last week, I need to update some of the class in order for the test to pass. I'm making progress
great_expectations/core/util.py:604: if not force_reuse_spark_context and spark_restart_required( great_expectations/data_context/types/base.py:672: force_reuse_spark_context = fields.Boolean(required=False, missing=False) great_expectations/data_context/types/base.py:792: force_reuse_spark_context = fields.Bool(required=False, allow_none=True) great_expectations/datasource/sparkdf_datasource.py:70: force_reuse_spark_context=False, great_expectations/datasource/sparkdf_datasource.py:103: "force_reuse_spark_context": force_reuse_spark_context, great_expectations/datasource/sparkdf_datasource.py:119: force_reuse_spark_context=False, great_expectations/datasource/sparkdf_datasource.py:136: force_reuse_spark_context, great_expectations/datasource/sparkdf_datasource.py:155: force_reuse_spark_context=force_reuse_spark_context, great_expectations/execution_engine/sparkdf_execution_engine.py:154: force_reuse_spark_context=False, great_expectations/execution_engine/sparkdf_execution_engine.py:165: force_reuse_spark_context=force_reuse_spark_context,

those path need to be updated, and that should hopefully fix the test.

@fep2
Copy link
Contributor Author

fep2 commented Oct 27, 2021

@talagluck I was able to fix the test woo hoo!

@fep2
Copy link
Contributor Author

fep2 commented Oct 27, 2021

Now some are failing boo, but they don't seem to be related to the MR.

Copy link
Contributor

@talagluck talagluck left a comment

Choose a reason for hiding this comment

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

Amazing - thanks so much for following up, and thanks for the contribution! LGTM!

@talagluck talagluck merged commit 677e84e into great-expectations:develop Nov 2, 2021
Shinnnyshinshin pushed a commit that referenced this pull request Nov 3, 2021
…-tests-for-docs

* develop:
  DevRel - docs fixes (#3498)
  Remove --no-spark flag from docs tests (#3625)
  [DOCS] Choosing and configuring DataConnectors (#3533)
  [BUGFIX] Be able to use spark execution engine with spark reuse flag (#3541)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community devrel This item is being addressed by the Developer Relations Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants