[PRO-16170] Give ability to set keys, create submission with cluster report, and … #99
Conversation
|
At first pass, the logic here is 👍🏻. My concern is that a lot of the method parameters have |
Agreed but we were trying to avoid making a drastic changes here while dealing with some legacy code that is used in the library today. |
| cluster: dict, | ||
| cluster_info: dict, | ||
| cluster_activity_events: dict, | ||
| tasks: List[dict], |
There was a problem hiding this comment.
I think these type hints need to be Dict to maintain the older python compatibility.
There was a problem hiding this comment.
Interesting. Theres a nice mix between the two already in the library. ill go with Dict though
There was a problem hiding this comment.
Oh hm, maybe it's only an issue when you try to type the contents of the dict. E.g. I'm almost certain that dict[str] would fail ... but maybe just a play dict is fine.
| if kill_on_termination: | ||
| cluster_state = get_default_client().get_cluster(cluster_id).get("state") | ||
| if cluster_state == "TERMINATED": | ||
| while_condition = False |
There was a problem hiding this comment.
Do we actually need the kill_on_termination flag at all? At a glance, it looks like if you did away with it but kept the check for TERMINATED then we're set for hosted monitoring and the existing functionality is unchanged (existing functionality being the monitoring running on the job cluster, in which case it's killed when the cluster terminates anyway).
There was a problem hiding this comment.
This is mostly a safeguard on changing behavior in existing functionality and not needing to check for the cluster state repeatedly from the init script. When we use hosted monitoring, we set kill_on_termination to true
| def set_databricks_config(db_config: DatabricksConf): | ||
| global _db_config | ||
| if _db_config is not None: | ||
| raise RuntimeError("Databricks config has already been set and the library does not support resetting " | ||
| "credentials") | ||
| _db_config = db_config |
| cluster: dict, | ||
| cluster_info: dict, | ||
| cluster_activity_events: dict, | ||
| tasks: List[dict], |
There was a problem hiding this comment.
Dict type hints here again.
gorskysd
left a comment
gorskysd
left a comment
There was a problem hiding this comment.
There are a few dict type hints that I think need to get changed to Dict to maintain compatibility prior to python 3.9. Also, might be able to clean up the monitoring flag, but not blocking on this. Otherwise looks good -- congratulations on navigating the hot mess of the library ;)
…expose cluster events
singhals
force-pushed
the
singhals/PROD-16170-override-cluster-report-submission-location
branch
from
114898c to
4335ae0
Compare
gorskysd
left a comment
gorskysd
left a comment
There was a problem hiding this comment.
Looks like the Dict change is unnecessary afterall, and it already exists elsewhere in the library. LGTM!
singhals
deleted the
singhals/PROD-16170-override-cluster-report-submission-location
branch
Summary
Checklist
Before formally opening this PR, please adhere to the following standards:
Related Jira Ticket (add id)
Add any relevant testing examples or screenshots.