PythonMutator: add diagnostics #1531

kanterov · 2024-06-26T13:44:26Z

Changes

Allow PyDABs to report dyn.Diagnostics by writing to diagnostics.json supplied as an argument, similar to input.json and output.json

Such errors are not yet properly printed in databricks bundle validate, which will be fixed in a follow-up PR.

Tests

Unit tests

codecov-commenter · 2024-06-26T13:52:46Z

Codecov Report

Attention: Patch coverage is 68.46154% with 41 lines in your changes missing coverage. Please review.

Project coverage is 54.01%. Comparing base (e22dd8a) to head (ca3bdfd).
Report is 168 commits behind head on main.

Files	Patch %	Lines
bundle/config/mutator/python/python_diagnostics.go	71.23%	15 Missing and 6 partials ⚠️
bundle/config/mutator/python/python_mutator.go	64.91%	14 Missing and 6 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1531      +/-   ##
==========================================
+ Coverage   52.25%   54.01%   +1.76%     
==========================================
  Files         317      354      +37     
  Lines       18004    20764    +2760     
==========================================
+ Hits         9408    11216    +1808     
- Misses       7903     8730     +827     
- Partials      693      818     +125

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bundle/config/mutator/python/python_diagnostics.go

kanterov · 2024-06-27T12:50:26Z

@pietern I've addressed your comments. Please take a look again.

bundle/config/mutator/python/python_mutator.go

shreyas-goenka · 2024-06-30T23:38:25Z

bundle/config/mutator/python/python_diagnostics.go

+	"github.com/databricks/cli/libs/dyn"
+)
+
+type pythonDiagnostic struct {


Could we serialize the diagnostics on the Python side in JSON instead of using the string representation for path and location?

That way we'll not need all the deserialization logic here to convert strings for location and path back to a diag.Dianostic. We could directly use json.Unmarshal.

cc: @pietern for thoughts on the serialzation format for diagnostics. Separately customers have asked for machine readable warnings, and the format we decide on here should be consistent with that.

I'd recommend the full JSON body because it's the simplest to maintain as well as is easy to parse for consumers of the JSON representation.

I did research on how other tools report warnings:

go vet

eslint

pylint

terraform validate

They use JSON for source locations.

All k8s tools report the path as a string. I will change it to do the same.

Also note that this format is purely internal, and we wouldn't show this to users, occasionally, we might need to read it for debugging.

If we ever want CLI to report warnings in a structured format, it can use any format not necessarily the same as here.

+1 -- I wouldn't want to couple them as they might need to evolve independently.

shreyas-goenka · 2024-06-30T23:41:44Z

bundle/config/mutator/python/python_mutator.go

@@ -116,7 +120,11 @@ func (m *pythonMutator) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagno
 		return merge.Override(leftRoot, rightRoot, visitor)
 	})

-	return diag.FromErr(err)
+	if len(mutateDiagnostics) != 0 {


Can we instead return a combined list of diagnostics here? Otherwise we could possibly ignore errors in the bundle.Mutate function if pyDABs returned a warning.

We can't merge errors because they can create duplicates. I will refactor them to make it easier to follow.

shreyas-goenka · 2024-06-30T23:46:09Z

bundle/config/mutator/python/python_mutator.go

@@ -152,42 +161,71 @@ func (m *pythonMutator) runPythonMutator(ctx context.Context, cacheDir string, r
 		inputPath,
 		"--output",
 		outputPath,
+		"--diagnostics",
+		diagnosticsPath,


Do we have a plan to keep the pyDABs version consistent with the CLI version? We don't need one immediately it'll likely be important down the line given there's an implicit dependency on the types defined in the CLI and pyDABs (diag.Diagnostics in this case).

It doesn't have to be consistent because changes need to be backward and forward-compatible. If we make a breaking change, we raise the minimum required version.

Where is the minimum required version stored/communicated?

We don't. For now, we can add a minimal required version of CLI as the configuration in the template, and document it.

We can also check in VerifyCliVersion, but right now, it's executed before we run the Python mutator if we want to append a different minimum required version there. Another difficulty is that this configuration parameter might already be used.

Another way is to pass the current CLI version into Python code and check it there. We can make it a part of the effective bundle configuration.

shreyas-goenka · 2024-06-30T23:54:55Z

bundle/config/mutator/python/python_mutator.go

+
+	// if diagnostics file exists, it gives the most descriptive errors
+	// if there is any error, we treat it as fatal error, and stop processing
+	if pythonDiagnostics.HasError() {


I'd recommend rolling up pythonDiagnosticsErr and processErr into the diagnostics that we return. We should protect against ignoring the errors if for some reason pythonDiagnostics do exist but they don't capture the actual error cause.

pythonDiagnosticsErr should always be nil if pythonDiagnostics has an error

The idea behind pythonDiagnostics is that we should never show a "process exit" error if it contains an explanation. This is how PyDABs explain what went wrong. There are 3 errors possible, and you can see the code below as the way for us to choose the most informative error that can explain what went wrong, in the order:

PyDABs reported an error that we loaded/parsed succesfuly

Process exited with non-zero code

We failed to load/parse the diagnostics file, but the process exited successfully

The error handling flow is not super straightforward. Could you include a few debug log lines to capture error conditions even if we don't act on them? When this goes wrong I'd like to be able to analyse why/how/etc.

Agree. I added logging just after each error is created

pietern · 2024-07-01T08:09:29Z

bundle/config/mutator/python/python_diagnostics.go

+	"github.com/databricks/cli/libs/dyn"
+)
+
+type pythonDiagnostic struct {


+1 -- I wouldn't want to couple them as they might need to evolve independently.

bundle/config/mutator/python/python_diagnostics.go

pietern · 2024-07-01T08:12:37Z

bundle/config/mutator/python/python_mutator.go

@@ -152,42 +161,71 @@ func (m *pythonMutator) runPythonMutator(ctx context.Context, cacheDir string, r
 		inputPath,
 		"--output",
 		outputPath,
+		"--diagnostics",
+		diagnosticsPath,


Where is the minimum required version stored/communicated?

pietern · 2024-07-01T08:18:14Z

bundle/config/mutator/python/python_mutator.go

+
+	// if diagnostics file exists, it gives the most descriptive errors
+	// if there is any error, we treat it as fatal error, and stop processing
+	if pythonDiagnostics.HasError() {


The error handling flow is not super straightforward. Could you include a few debug log lines to capture error conditions even if we don't act on them? When this goes wrong I'd like to be able to analyse why/how/etc.

pietern · 2024-07-01T08:18:38Z

bundle/config/mutator/python/python_mutator.go

+//
+// It contains a list of warnings and errors that we should print to users.
+func loadDiagnosticsFile(path string) (diag.Diagnostics, error) {
+	file, err := os.Open(path)


Is it guaranteed to exist, even if there are no diagnostics?

Yes, I think it will be less error-prone like that, so it doesn't happen that integration suddenly breaks and stops to print warnings and errors

Please capture this intent in the comment. The next person looking at this code could add a not-exists check not being aware of this intent.

pietern · 2024-07-01T08:19:05Z

bundle/config/mutator/python/python_mutator_test.go


-	assert.NoError(t, diag.Error())
+	assert.NoError(t, diagnostics.Error())


Convention: diags

kanterov · 2024-07-01T10:42:05Z

@shreyas-goenka @pietern addressed your comments, please take a look.

The remaining question is the CLI version check. I don't find it pressing if we put effort into templates and documentation. I think in the long run, we should serialize the current CLI version as a part of the JSON we pass. What do you think?

pietern

One minor last comment

pietern · 2024-07-02T08:13:56Z

bundle/config/mutator/python/python_mutator.go

+//
+// It contains a list of warnings and errors that we should print to users.
+func loadDiagnosticsFile(path string) (diag.Diagnostics, error) {
+	file, err := os.Open(path)


Please capture this intent in the comment. The next person looking at this code could add a not-exists check not being aware of this intent.

kanterov · 2024-07-02T11:00:45Z

@pietern done, I've added an elaborate comment and test. Can you please add it to the merge queue?

@pietern @shreyas-goenka thanks for taking the look

Bundles: As of this release you can interact with bundles when running the CLI on DBR (e.g. via the Web Terminal). * Fix non-default project names not working in dbt-sql template ([#1500](#1500)). * Improve `bundle validate` output ([#1532](#1532)). * Fixed resolving variable references inside slice variable ([#1550](#1550)). * Fixed bundle not loading when empty variable is defined ([#1552](#1552)). * Use `vfs.Path` for filesystem interaction ([#1554](#1554)). * Replace `vfs.Path` with extension-aware filer when running on DBR ([#1556](#1556)). Internal: * merge.Override: Fix handling of dyn.NilValue ([#1530](#1530)). * Compare `.Kind()` instead of direct equality checks on a `dyn.Value` ([#1520](#1520)). * PythonMutator: register product in user agent extra ([#1533](#1533)). * Ignore `dyn.NilValue` when traversing value from `dyn.Map` ([#1547](#1547)). * Add extra tests for the sync block ([#1548](#1548)). * PythonMutator: add diagnostics ([#1531](#1531)). * PythonMutator: support omitempty in PyDABs ([#1513](#1513)). * PythonMutator: allow insert 'resources' and 'resources.jobs' ([#1555](#1555)).

PythonMutator: add diagnostics

ca3bdfd

kanterov force-pushed the python-mutator-diagnostics branch from fa8d143 to ca3bdfd Compare June 26, 2024 13:50

kanterov requested a review from pietern June 26, 2024 13:53

pietern reviewed Jun 27, 2024

View reviewed changes

bundle/config/mutator/python/python_diagnostics.go Outdated Show resolved Hide resolved

Use json.NewDecoder instead of scanner

cf32e36

kanterov requested a review from pietern June 27, 2024 10:59

shreyas-goenka reviewed Jul 1, 2024

View reviewed changes

Address CR comments

6442bc7

pietern reviewed Jul 1, 2024

View reviewed changes

kanterov added 2 commits July 1, 2024 12:34

Address CR comments per pietern

4bb07a4

Fix fmt

8f6a597

kanterov requested review from pietern and shreyas-goenka July 1, 2024 10:40

pietern approved these changes Jul 2, 2024

View reviewed changes

kanterov and others added 2 commits July 2, 2024 11:08

Clarify loadDiagnosticsFile behavior

268f6fe

Merge branch 'main' into python-mutator-diagnostics

1b41c4e

pietern added this pull request to the merge queue Jul 2, 2024

Merged via the queue into databricks:main with commit 5a0a6d7 Jul 2, 2024
5 checks passed

pietern mentioned this pull request Jul 3, 2024

Release v0.223.0 #1557

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PythonMutator: add diagnostics #1531

PythonMutator: add diagnostics #1531

kanterov commented Jun 26, 2024

codecov-commenter commented Jun 26, 2024 •

edited

Loading

kanterov commented Jun 27, 2024

shreyas-goenka Jun 30, 2024

shreyas-goenka Jun 30, 2024

kanterov Jul 1, 2024 •

edited

Loading

kanterov Jul 1, 2024

pietern Jul 1, 2024

shreyas-goenka Jun 30, 2024

kanterov Jul 1, 2024

shreyas-goenka Jun 30, 2024

kanterov Jul 1, 2024

pietern Jul 1, 2024

kanterov Jul 1, 2024

shreyas-goenka Jun 30, 2024

kanterov Jul 1, 2024

pietern Jul 1, 2024

kanterov Jul 1, 2024

pietern Jul 1, 2024

pietern Jul 1, 2024

pietern Jul 1, 2024

pietern Jul 1, 2024

kanterov Jul 1, 2024

pietern Jul 2, 2024

pietern Jul 1, 2024

kanterov commented Jul 1, 2024

pietern left a comment

pietern Jul 2, 2024

kanterov commented Jul 2, 2024


		assert.NoError(t, diag.Error())
		assert.NoError(t, diagnostics.Error())

PythonMutator: add diagnostics #1531

PythonMutator: add diagnostics #1531

Conversation

kanterov commented Jun 26, 2024

Changes

Tests

codecov-commenter commented Jun 26, 2024 • edited Loading

Codecov Report

kanterov commented Jun 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanterov Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanterov commented Jul 1, 2024

pietern left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanterov commented Jul 2, 2024

codecov-commenter commented Jun 26, 2024 •

edited

Loading

kanterov Jul 1, 2024 •

edited

Loading