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

fix typos in DatabricksSubmitRunOperator #36248

Merged
merged 3 commits into from
Dec 21, 2023
Merged

fix typos in DatabricksSubmitRunOperator #36248

merged 3 commits into from
Dec 21, 2023

Conversation

adam133
Copy link
Contributor

@adam133 adam133 commented Dec 15, 2023

Finally got around to trying this, realized I had a couple major typos in my original PR. Works to execute by pipeline name after these two changes.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Copy link
Contributor

@josh-fell josh-fell left a comment

Choose a reason for hiding this comment

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

Would you mind adding tests for the DatabricksHook to cover this for the future please?

Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

Since this bug was not detected in the tests, IMHO, we need to update them to avoid reintroducing it in the future. Could you please add a new test or update an existing one to cover this call?

@@ -521,7 +521,7 @@ def execute(self, context: Context):
):
# If pipeline_id is not provided, we need to fetch it from the pipeline_name
pipeline_name = self.json["pipeline_task"]["pipeline_name"]
self.json["pipeline_task"]["pipeline_id"] = self._hook.get_pipeline_id(pipeline_name)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why Mypy didn't block #32903 which added this call to the non-existing method. @potiuk do you have an idea?

Copy link
Member

Choose a reason for hiding this comment

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

It's a mistery . MyPy uses a number of heuristics to find types of particular objects and sometime it gives up or sometimes drop types if they are too complex to analyse. Generally MyPy's behaveriour is often non-deterministic/difficult to reason about.

@adam133
Copy link
Contributor Author

adam133 commented Dec 18, 2023

Would you mind adding tests for the DatabricksHook to cover this for the future please?

I can try, but it'll take me a bit to understand how the testing code works. Might need some pointers

@potiuk
Copy link
Member

potiuk commented Dec 18, 2023

I can try, but it'll take me a bit to understand how the testing code works. Might need some pointers

Best to look at other, neighbouring tests and do it in s similar way. TESTING.rst contains detailed description of different kinds of tests we have and how to run them locally

@adam133
Copy link
Contributor Author

adam133 commented Dec 20, 2023

I can try, but it'll take me a bit to understand how the testing code works. Might need some pointers

Best to look at other, neighbouring tests and do it in s similar way. TESTING.rst contains detailed description of different kinds of tests we have and how to run them locally

Alright, took a bit but I got some good tests in there. Found a few more bugs along the way.

@potiuk potiuk merged commit 322aa64 into apache:main Dec 21, 2023
52 checks passed
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.

4 participants