[Python] Move 'python' outside of experimental #3540
Conversation
kanterov
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
kanterov
force-pushed
the
python-non-experimental
branch
from
fe939e5 to
3c1043c
Compare
kanterov
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
40 failing tests:
|
kanterov
requested review from
pietern,
andrewnester,
shreyas-goenka,
denik and
anton-107
as code owners
| [EnvMatrix] | ||
| DATABRICKS_CLI_DEPLOYMENT = ["terraform", "direct-exp"] | ||
| UV_ARGS = ["--with databricks-bundles==0.7.3", "--with-requirements requirements-latest.txt --no-cache"] | ||
| UV_ARGS = ["--with databricks-bundles==0.266.0", "--with-requirements requirements-latest.txt --no-cache"] |
There was a problem hiding this comment.
Is it intentional to test only 0.266.0? What about the most recent release or main?
There was a problem hiding this comment.
We need some version from the past, and it was selected to be 0.266.0. It doesn't have to be the latest release. We just need to select a version that users might pin.
The second case for UV_ARGS is testing against the latest version from main
There was a problem hiding this comment.
Makes sense to test past version -- please add a comment that this is intentional.
There was a problem hiding this comment.
Does UV_ARGS="--with-requirements requirements-latest.txt --no-cache" results in testing the python package from the current branch or latest release from PyPI. Please add a comment as well, it's not clear.
There was a problem hiding this comment.
👍 , I've clarified. It's using the same commit as acceptance tests
| // don't execute for phases for 'pydabs' section | ||
| if phase == PythonMutatorPhaseLoadResources || phase == PythonMutatorPhaseApplyMutators { | ||
| if experimentalPythonEnabled && pythonEnabled { | ||
| if !reflect.DeepEqual(experimental.Python, b.Config.Python) { |
There was a problem hiding this comment.
why reflect.Equal? Why not require users specify this once?
There was a problem hiding this comment.
I see below that this is to make python package work but IMO we should separate what we require from users and what we send to Python. E.g. we can have validator that requires that only one is present but then duplicate and send both to Python.
There was a problem hiding this comment.
The first time we call "getOpts", only 1 property can be specified. The second time, we call "getOpts" on the mutated bundle that contains both options specified after we have called applyBackwardsCompatibilityFixes, so we need to allow this case as well.
Here, we call applyBackwardsCompatibilityFixes before getOpts, so even the first time, we expect both options to be set.
There was a problem hiding this comment.
why do we call it twice? cannot we remember the output from the first call?
There was a problem hiding this comment.
Our mutators are intended to be stateless, so we can't remember. And we call Python mutator twice in the mutator pipeline
There was a problem hiding this comment.
Our mutators are intended to be stateless, so we can't remember. And we call Python mutator twice in the mutator pipeline
You can certainly have a field on *Bundle to track state relevant for you. Especially if it saves work and simplifies code.
There was a problem hiding this comment.
Right. I just don't see a problem. If we want the bundle spec to be stable, we should allow specifying both properties during the transition period.
| mutators: | ||
| - "mutators:add_task_1" | ||
| - "mutators:add_task_2" | ||
| python: |
There was a problem hiding this comment.
Would be good to have acceptance tests for new functionality:
- only experimental.python is provided
- both are provided and equal
- both are provided and not equal
There was a problem hiding this comment.
👍 , added in the latest commit
kanterov
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
kanterov
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
## Changes Support reading both `python` and `experimental.python` configs. If both are set, their value should be equivalent. After we introduce the `python` section into the CLI, it has to keep setting both `python` and `experimental.python` into the payload for backward compatibility. After that, Python code can start erroring out if `experimental/python` is set but not `python` because it means that users have an outdated CLI version. See also #3540 ## Why It's a step toward graduating `python` outside the `experimental` section. ## Tests Unit tests and existing acceptance tests
kanterov
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
kanterov
force-pushed
the
python-non-experimental
branch
from
d77a490 to
f751173
Compare
kanterov
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Changes
Move
pythonoutside of theexperimentalsection.The change is backward-compatible and forward-compatible. If CLI is using a newer or older version of
databricks-bundles,it will continue to work. It's implemented by copying the content ofpythonintoexperimental.python. If we don't offer compatibility, there is a risk that previously deployed resources will be erased because we wouldn't be able to load resources.See also #3539
Why
Python support for Databricks Asset Bundles is graduating from the experimental status.
Tests
Acceptance tests check that previous
databricks-bundlesversions continue to work.