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] For DataAssistants, added try-except for Notebook tests #5124
[BUGFIX] For DataAssistants, added try-except for Notebook tests #5124
Conversation
✅ Deploy Preview for niobium-lead-7998 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
"#import shutil, os\n", | ||
"#shutil.rmtree(\"great_expectations/expectations/tmp\")\n", | ||
"#os.remove(\"great_expectations/expectations/.ge_store_backend_id\")\n", | ||
"#os.remove(\"great_expectations/expectations/taxi_data_suite.json\")" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out this was the cause of the bug 😓
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor changes requested - if we move the file deletion to the finally
block, we will ensure that those files are always deleted (regardless of errors or exceptions).
try: | ||
shutil.rmtree(os.path.join(base_dir, "great_expectations/expectations/tmp")) | ||
os.remove( | ||
os.path.join( | ||
base_dir, "great_expectations/expectations/.ge_store_backend_id" | ||
) | ||
) | ||
except FileNotFoundError: | ||
# This just means the files were already deleted by running the optional last cell in the notebook. | ||
# Therefore we allow the test to pass. | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this file deletion should occur nested under the finally
block - if anything goes wrong earlier on in the test, these files will not be deleted.
Also, do we need to write the results to a temp notebook? What are your thoughts on devs just running the original notebook to debug?
Finally, could we please log when we hit the FileNotFoundError
so that comment you wrote is presented in stdout?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestions.I thought the point on adding the FileNotFoundError
to the log was fantastic, so my most recent commit has that change. I have some questions about what you had in mind for finally
block and was wondering if you had time to discuss tomorrow?
try: | ||
shutil.rmtree(os.path.join(base_dir, "great_expectations/expectations/tmp")) | ||
os.remove( | ||
os.path.join( | ||
base_dir, "great_expectations/expectations/.ge_store_backend_id" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same notes here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for helping me see the light ⚡ 🌞
…ing-expectations * develop: [FEATURE] `DataAssistant` plotting for non-sequential batches (#5126) Move upper bound on sqlalchemy to <2.0.0. (#5140) [FEATURE] Enable self-initializing `ExpectColumnValueLengthsToBeBetween` (#4985) Use `external_sqldialect` mark to skip tests during lightweight packaging test run. (#5139) [FEATURE] For OnboardingDataAssistant: Implement a TABLE Domain level rule to output "expect_table_columns_to_match_set" (#5137) [FEATURE] Giving the "expect_table_columns_to_match_set" Expectation Self-Initializing Capabilities. (#5136) [FEATURE] OnboardingDataAssistant: Introduce MeanTableColumnsSetMatchMultiBatchParameterBuilder (to enable expect_table_columns_to_match_set) (#5135) [FEATURE] Categorical Rule is added to OnboardingDataAssistant (#5134) CategoricalColumnDomainBuilder needs to accept limit_mode with dictionary type (#5127) add trino dialect (#5085) [BUGFIX] For DataAssistants, added try-except for Notebook tests (#5124) [MAINTENANCE] Sqlite specific tests for splitting and sampling (#5119) [FEATURE] DateTime Rule for OnboardingDataAssistant (#5121) [FEATURE] Onboarding DataAssistant: Numeric Rules and Relevant Metrics (#5120) [BUGFIX] Update `DataAssistant` notebook for new plotting API (#5118)
What went wrong?
RBP
andDataAssistant
Notebook that deletes files that are usually generated by the notebook (like ExpectationSuites, and.ge_cloud_id
files). This cell is normally commented out, but wasn't in the previous PR 😅test_run_profiler_notebook.py
tests, the is a part of the test that tries to delete these same files.. since they were deleted by running the Notebook, the test was giving aFileNotFoundError
.What does this PR fix?
DataAssistant
Notebook that was causing tests to failtests/integration/profiling/rule_based_profilers/test_run_profiler_notebook.py
Thank you for submitting!