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

Pass parameters to task when run with --python-params and python_wheel_wrapper is true #1037

Merged
merged 2 commits into from
Dec 1, 2023

Conversation

andrewnester
Copy link
Contributor

@andrewnester andrewnester commented Nov 30, 2023

Changes

It makes the behaviour consistent with or without python_wheel_wrapper on when job is run with --python-params flag.

In python_wheel_wrapper mode it converts dynamic python_params in a dynamic specially named notebook_param and the wrapper reads them with dbutils and pass to sys.argv

Fixes #1000

Tests

Added an integration test.

Integration tests pass.

Copy link
Contributor

@pietern pietern 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!

if python_params:
params = json.loads(python_params)
except Exception as e:
print(e)
Copy link
Contributor

Choose a reason for hiding this comment

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

This can mess with stdout; how about letting this bubble up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pietern the reason I'm not re-raising this is that dbutils.widgets.get("__python_params") throws an exception when no python params passed, so we just log it. Since we're buffering Python wheel output below, it should not impact the stdout there.

Copy link
Contributor

@pietern pietern Dec 1, 2023

Choose a reason for hiding this comment

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

What kind of exception is thrown if the parameter doesn't exist? Could we make the exception we catch narrower?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pietern it's Py4JJavaError, not sure if it's specific enough to be helpful in this case

bundle/run/job.go Show resolved Hide resolved
@pietern pietern changed the title Pass parameters to task when run with --python-params and python_wheel_wrapper is true Pass parameters to task when run with --python-params and python_wheel_wrapper is true Dec 1, 2023
@pietern pietern changed the title Pass parameters to task when run with --python-params and python_wheel_wrapper is true Pass parameters to task when run with --python-params and python_wheel_wrapper is true Dec 1, 2023
@andrewnester andrewnester added this pull request to the merge queue Dec 1, 2023
Merged via the queue into main with commit 83d5000 Dec 1, 2023
@andrewnester andrewnester deleted the fix-python-params branch December 1, 2023 10:40
andrewnester added a commit that referenced this pull request Dec 6, 2023
CLI:
 * Add documentation for positional args in commands generated from the Databricks OpenAPI specification ([#1033](#1033)).
 * Ask for host when .databrickscfg doesn't exist ([#1041](#1041)).
 * Add list of supported values for flags that represent an enum field ([#1036](#1036)).

Bundles:
 * Fix panic when bundle auth resolution fails ([#1002](#1002)).
 * Add versioning for bundle templates ([#972](#972)).
 * Add support for conditional prompting in bundle init ([#971](#971)).
 * Pass parameters to task when run with `--python-params` and `python_wheel_wrapper` is true ([#1037](#1037)).
 * Change default_python template to auto-update version on each wheel build ([#1034](#1034)).

Internal:
 * Rewrite the friendly log handler ([#1038](#1038)).
 * Move bundle schema update to an internal module ([#1012](#1012)).

Dependency updates:
 * Bump github.com/databricks/databricks-sdk-go from 0.26.0 to 0.26.1 ([#1040](#1040)).
@andrewnester andrewnester mentioned this pull request Dec 6, 2023
github-merge-queue bot pushed a commit that referenced this pull request Dec 6, 2023
CLI:
* Add documentation for positional args in commands generated from the
Databricks OpenAPI specification
([#1033](#1033)).
* Ask for host when .databrickscfg doesn't exist
([#1041](#1041)).
* Add list of supported values for flags that represent an enum field
([#1036](#1036)).

Bundles:
* Fix panic when bundle auth resolution fails
([#1002](#1002)).
* Add versioning for bundle templates
([#972](#972)).
* Add support for conditional prompting in bundle init
([#971](#971)).
* Pass parameters to task when run with `--python-params` and
`python_wheel_wrapper` is true
([#1037](#1037)).
* Change default_python template to auto-update version on each wheel
build ([#1034](#1034)).

Internal:
* Rewrite the friendly log handler
([#1038](#1038)).
* Move bundle schema update to an internal module
([#1012](#1012)).


Dependency updates:
* Bump github.com/databricks/databricks-sdk-go from 0.26.0 to 0.26.1
([#1040](#1040)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bundle run fails for non unity-catalog workspace
2 participants