Skip to content

Commit d3ac142

Browse files
authored
[Python] Support non-experimental conf in Python code (#3539)
## 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
1 parent 54cdadd commit d3ac142

File tree

4 files changed

+170
-7
lines changed

4 files changed

+170
-7
lines changed

experimental/python/databricks/bundles/build.py

Lines changed: 48 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -166,16 +166,12 @@ def _apply_mutators_for_type(
166166
return resources, Diagnostics()
167167

168168

169-
def python_mutator(
170-
args: _Args,
171-
) -> tuple[dict, dict[tuple[str, ...], Location], Diagnostics]:
172-
input = json.load(open(args.input, encoding="utf-8"))
169+
def _read_conf(input: dict) -> tuple[_Conf, Diagnostics]:
173170
experimental = input.get("experimental", {})
174171

175172
if experimental.get("pydabs", {}) != {}:
176173
return (
177-
{},
178-
{},
174+
_Conf(),
179175
Diagnostics.create_error(
180176
"'experimental/pydabs' is not supported by 'databricks-bundles', use 'experimental/python' instead",
181177
detail="",
@@ -184,8 +180,53 @@ def python_mutator(
184180
),
185181
)
186182

187-
conf_dict = experimental.get("python", {})
183+
experimental_conf_dict = experimental.get("python", {})
184+
experimental_conf = _transform(_Conf, experimental_conf_dict)
185+
186+
conf_dict = input.get("python", {})
188187
conf = _transform(_Conf, conf_dict)
188+
189+
has_conf = conf != _Conf()
190+
has_experimental_conf = experimental_conf != _Conf()
191+
192+
if has_conf and not has_experimental_conf:
193+
return conf, Diagnostics()
194+
elif not has_conf and has_experimental_conf:
195+
# do not generate warning in Python code, if CLI supports non-experimental 'python',
196+
# it should generate a warning
197+
return experimental_conf, Diagnostics()
198+
elif has_conf and has_experimental_conf:
199+
# for backward-compatibility, CLI can copy contents of 'python' into 'experimental/python'
200+
# if configs are equal, it isn't a problem
201+
if conf != experimental_conf:
202+
return (
203+
_Conf(),
204+
Diagnostics.create_error(
205+
"Both 'python' and 'experimental/python' sections are present, use 'python' section only",
206+
detail="",
207+
location=None,
208+
path=(
209+
"experimental",
210+
"python",
211+
),
212+
),
213+
)
214+
else:
215+
return conf, Diagnostics()
216+
else:
217+
return _Conf(), Diagnostics()
218+
219+
220+
def python_mutator(
221+
args: _Args,
222+
) -> tuple[dict, dict[tuple[str, ...], Location], Diagnostics]:
223+
input = json.load(open(args.input, encoding="utf-8"))
224+
diagnostics = Diagnostics()
225+
226+
conf, diagnostics = diagnostics.extend_tuple(_read_conf(input))
227+
if diagnostics.has_error():
228+
return input, {}, diagnostics
229+
189230
bundle = _parse_bundle_info(input)
190231

191232
if args.phase == "load_resources":

experimental/python/databricks/bundles/core/_diagnostics.py

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,17 @@ def has_error(self) -> bool:
134134

135135
return False
136136

137+
def has_warning(self) -> bool:
138+
"""
139+
Returns True if there is at least one warning in diagnostics.
140+
"""
141+
142+
for item in self.items:
143+
if item.severity == Severity.WARNING:
144+
return True
145+
146+
return False
147+
137148
@classmethod
138149
def create_error(
139150
cls,

experimental/python/databricks_tests/core/test_diagnostics.py

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,3 +85,17 @@ def test_location_from_weird_callable():
8585
location = Location.from_callable(print)
8686

8787
assert location is None
88+
89+
90+
def test_has_error():
91+
diagnostics = Diagnostics.create_error("foo is deprecated")
92+
93+
assert diagnostics.has_error()
94+
assert not diagnostics.has_warning()
95+
96+
97+
def test_has_warning():
98+
diagnostics = Diagnostics.create_warning("foo is deprecated")
99+
100+
assert not diagnostics.has_error()
101+
assert diagnostics.has_warning()

experimental/python/databricks_tests/test_build.py

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
_load_resources_from_input,
1414
_parse_args,
1515
_parse_bundle_info,
16+
_read_conf,
1617
_relativize_location,
1718
_write_diagnostics,
1819
_write_locations,
@@ -487,3 +488,99 @@ def load_resources_2() -> Resources:
487488
file="my_job_2.py", line=42, column=1
488489
),
489490
}
491+
492+
493+
def test_parse_conf_empty():
494+
actual, diagnostics = _read_conf({})
495+
496+
assert actual == _Conf()
497+
assert diagnostics == Diagnostics()
498+
499+
500+
def test_parse_conf_both_equal():
501+
conf, diagnostics = _read_conf(
502+
{
503+
"python": {
504+
"venv_path": "venv",
505+
"resources": ["my_package:load_resources"],
506+
"mutators": ["my_package:mutator_1"],
507+
},
508+
"experimental": {
509+
"python": {
510+
"venv_path": "venv",
511+
"resources": ["my_package:load_resources"],
512+
"mutators": ["my_package:mutator_1"],
513+
},
514+
},
515+
}
516+
)
517+
518+
assert not diagnostics.has_warning()
519+
assert not diagnostics.has_error()
520+
assert conf == _Conf(
521+
venv_path="venv",
522+
resources=["my_package:load_resources"],
523+
mutators=["my_package:mutator_1"],
524+
)
525+
526+
527+
def test_parse_conf_both_incompatible():
528+
_, diagnostics = _read_conf(
529+
{
530+
"python": {
531+
"venv_path": "venv",
532+
"resources": ["my_package:load_resources"],
533+
"mutators": ["my_package:mutator_1"],
534+
},
535+
"experimental": {
536+
"python": {
537+
"venv_path": "venv",
538+
"resources": ["my_package:load_resources"],
539+
},
540+
},
541+
}
542+
)
543+
544+
assert not diagnostics.has_warning()
545+
assert diagnostics.has_error()
546+
547+
548+
def test_parse_conf_experimental():
549+
conf, diagnostics = _read_conf(
550+
{
551+
"experimental": {
552+
"python": {
553+
"venv_path": "venv",
554+
"resources": ["my_package:load_resources"],
555+
"mutators": ["my_package:mutator_1"],
556+
},
557+
},
558+
}
559+
)
560+
561+
assert not diagnostics.has_warning()
562+
assert not diagnostics.has_error()
563+
assert conf == _Conf(
564+
venv_path="venv",
565+
resources=["my_package:load_resources"],
566+
mutators=["my_package:mutator_1"],
567+
)
568+
569+
570+
def test_parse_conf():
571+
conf, diagnostics = _read_conf(
572+
{
573+
"python": {
574+
"venv_path": "venv",
575+
"resources": ["my_package:load_resources"],
576+
"mutators": ["my_package:mutator_1"],
577+
},
578+
}
579+
)
580+
581+
assert diagnostics == Diagnostics()
582+
assert conf == _Conf(
583+
venv_path="venv",
584+
resources=["my_package:load_resources"],
585+
mutators=["my_package:mutator_1"],
586+
)

0 commit comments

Comments
 (0)