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

[CT-2387] [Feature] Always write run_results.json, even if --fail-fast or unhandled error #7302

Closed
Tracked by #7301
jtcohen6 opened this issue Apr 10, 2023 · 4 comments · Fixed by #7539
Closed
Tracked by #7301
Assignees
Labels
artifacts enhancement New feature or request

Comments

@jtcohen6
Copy link
Contributor

jtcohen6 commented Apr 10, 2023

As a consumer of dbt metadata, or a person who wants to "retry" a failed command (#7299), I need run_results.json to always be available at the end of a command.

Currently, run_results.json is not written if:

  • The run is cancelled partway through (e.g. process termination). Even if we'd already run the vast majority of nodes over several hours, that progress is "lost."
  • --fail-fast, which has the effect of gracelessly terminating the process / cancelling nodes in other threads, as soon as one failure is encountered.

Discussing with @ChenyuLInx, an answer that doesn't feel so terrible: write run_results.json after every node completes. That way, we don't need to gracefully handle graceless exits (whether due to --fail-fast, keyboard interrupt, process termination...)

Originally posted by @jtcohen6 in #3600 (comment)

@github-actions github-actions bot changed the title Always write run_results.json, even if --fail-fast or unhandled error [CT-2387] Always write run_results.json, even if --fail-fast or unhandled error Apr 10, 2023
@jtcohen6 jtcohen6 changed the title [CT-2387] Always write run_results.json, even if --fail-fast or unhandled error [CT-2387] [Feature] Always write run_results.json, even if --fail-fast or unhandled error Apr 10, 2023
@iknox-fa iknox-fa added the Refinement Maintainer input needed label Apr 10, 2023
@iknox-fa
Copy link
Contributor

iknox-fa commented Apr 10, 2023

@jtcohen6
We'd like to break out the failure of fail-fast to write run_results.json into a separate ticket-- it feels very much like a bug in how that code works. We'd also like some further AC on exactly when we should persist run_results.json to disk.

We're going to point it based on the following assumptions:

  • We write when we get all of the inputs to the run (essentially writing flags to disk)
  • We write on the completion of every node
  • We write at the end (specifically to address tasks that don't execute the dag)

@jtcohen6
Copy link
Contributor Author

@iknox-fa All of that makes sense to me!

I'd be happy to reopen #3600, and keep it open as a separate/standalone issue, since that one was initially scoped to / motivated by the --fail-fast case.

@jtcohen6 jtcohen6 removed the Refinement Maintainer input needed label Apr 24, 2023
@iknox-fa
Copy link
Contributor

iknox-fa commented Apr 24, 2023

@jtcohen6 Per BLG on 4/24-- It turns out that the original ticket (#3600) was fixed in some other PR between July of '21 and now. That said, a keyboard based process termination does still fail to write run_results.json so we've made a new ticket to spiek the correct way of fixing that rather than re-write 3600 to address the other use case.

#7448

@jtcohen6
Copy link
Contributor Author

@iknox-fa Makes sense to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
artifacts enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants