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

[MAINTENANCE] performance, avoid HTTP round trips #9929

Merged
merged 18 commits into from
May 22, 2024

Conversation

dctalbot
Copy link
Contributor

@dctalbot dctalbot commented May 15, 2024

  • Currently, when building the evaluation parameters, N round trips are made to the GX Cloud store for N Expectation Suites.
  • This PR avoids this by implementing the get_all method on the ExpectationsStore's interface.
  • I suggest using the "hide whitespace changes" option when reviewing the code diff


@override
@staticmethod
def gx_cloud_response_json_to_object_collection(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

see great_expectations/data_context/store/datasource_store.py for reference implementation

Copy link
Member

@Kilo59 Kilo59 left a comment

Choose a reason for hiding this comment

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

Nice work! It's hard to overstate the impact this will have 😄

Approach seems reasonable I would suggest using the _fake_web_api mock to assert that the expected number of calls are being made to /expectations-suites + /expectations-suites?name=<SUITE-NAME>

I think you'll need to take the PR off of draft mode to run the integration tests.

@dctalbot dctalbot marked this pull request as ready for review May 15, 2024 13:05
Copy link
Contributor

@tyler-hoffman tyler-hoffman left a comment

Choose a reason for hiding this comment

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

Looks good to me. the sa _get_all is a mouthful, but looks consistent with the rest of the class. Happy to approve once tests for the other backends are ported over!

)

checkpoint.run()
# TODO assert GET expectation-suites called
Copy link
Member

@Kilo59 Kilo59 May 21, 2024

Choose a reason for hiding this comment

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

Did you mean to do this?

I'm working on this right now.

Copy link
Member

Choose a reason for hiding this comment

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

This is turning out to be much more of a pain to setup than I expected.
Let's do this as a followup.

@Kilo59 Kilo59 enabled auto-merge May 22, 2024 14:52
@Kilo59 Kilo59 added this pull request to the merge queue May 22, 2024
Merged via the queue into 0.18.x with commit 6d1d807 May 22, 2024
56 checks passed
@Kilo59 Kilo59 deleted the m/PH-1037/fix-ch-run-reqs branch May 22, 2024 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants