-
Notifications
You must be signed in to change notification settings - Fork 99
Semantic model perf lab #426
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
Conversation
return str(self._properties) | ||
|
||
|
||
def _get_or_create_workspace( |
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.
should move to common utils.
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.
Following up offline with Michael.
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.
Moving this to _helper_functions.py
create_abfss_path, | ||
) | ||
|
||
class PropertyBag: |
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.
why not just dict?
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.
Yeah, dict is much easier. Replaced.
raise ValueError("For new workspaces, the workspace parameter must be string, not a Guid. Please provide a workspace name.") | ||
except ValueError: | ||
# OK, it's not a Guid. But also make sure the workspace parameter isn't empty. | ||
if workspace == "" or workspace is None: |
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.
just use if workspace:
https://stackoverflow.com/questions/9573244/how-to-check-if-the-string-is-empty-in-python
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. Changed throughout.
print(f"{icons.green_dot} Workspace '{workspace_name}' created.") | ||
return (workspace,workspace_id) | ||
|
||
def _get_or_create_lakehouse( |
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.
move to utility file
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.
Following up offline with Michael.
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.
Moving this to _helper_functions.py
""" | ||
|
||
# Treat empty strings as None. | ||
if lakehouse == "": |
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.
not sure why this is needed.
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.
No longer in the sample_lab file..
|
||
try: | ||
(target_workspace_name, target_workspace_id) = resolve_workspace_name_and_id(workspace=target_workspace) | ||
except: |
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.
catch specific exception, otherwise we mask errors
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.
Thanks. Replaced with (WorkspaceNotFoundException, ValueError).
|
||
return spark.createDataFrame(rows, schema=schema).dropDuplicates() | ||
|
||
def _get_min_max_keys( |
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.
move to util file
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.
Following up offline with Michael.
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.
Michael already has a _get_column_aggregate utility function which does essentially the same thing, but it doesn't take a table path as a parameter and only returns a single int value. For now, let's leave it here. Happy to move later if OK with Michael.
def get_storage_table_column_segments( | ||
test_cycle_definitions: DataFrame, | ||
tables_info: DataFrame | ||
)->DataFrame: |
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.
run black to get formatting
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.
Done! File reformatted.
|
||
try: | ||
(target_workspace_name, target_workspace_id) = resolve_workspace_name_and_id(workspace=target_workspace) | ||
except: |
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.
catch specific exception
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.
Fixed (WorkspaceNotFoundException, ValueError)
] | ||
|
||
|
||
def _get_test_definitions( |
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.
the dataframe seems small, so I'd suggest not to use a pandas dataframe.
since you want to subsequently persist it (and maybe re-hydrate), I think a separate class would make it more convenient.
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.
Agreed. Classes are more user friendly and less error prone. Also let me hard-code the fields that the test definitions must have. But I do want to keep this flexible so that users can add more fields as needed for their specific cases. For example, a query Category can be useful for analysis but it's not strictly needed for the perf lab. Long story short, added TestDefinition and TestSuite classes that feature the standard fields but also persist and load any additional fields the user may want to have.
olEPs = response.json().get("oneLakeEndpoints") | ||
dfsEP = olEPs.get("dfsEndpoint") | ||
|
||
start_expr = "let\n\tdatabase = " |
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.
multiline string using """
""" to improve readability
Closing this big PR for a series of small PRs, as discussed offline. |
Functions to provision and run a Semantic Model Perf Lab for continuous testing and ad-hoc investigations.