Repository navigation
Remove trace error introduced by tune bump and fix upload bug - #551
Conversation
wagnerlmichael
changed the title
wagnerlmichael
changed the title
tune bump
There was a problem hiding this comment.
Alright, to make sure I'm understanding:
We've got two errors caused by the .notes column: one we already address in condos and we're addressing for the res avm here that is essentially us aggregating notes across iterations before uploading them to S3.
The second is a new column caused by an update in tune that added a new column for debugging which we're just dropping here.
This seems fine to me... I don't think we need to persist this column in s3 and athena. But, could we test printing the notes before this column is removed so that we can see it in cloudwatch if there's ever a tuning error?
@jeancochrane you might want to put eyes on this as well to confirm I'm thinking this through correctly.
That's in line with my understanding!
I'll check it out, I'm currently investigating the contents of the trace column after Jean took a brief look at this PR and messaged me out of band |
|
Some notes on the trace column and an alternative implementation: This would have been my best shot at including the trace in the athena cv table. I think Billy's proposition is better for a few resons:
# Flattening each rlang trace to plain text keeps the diagnostic content in a
# form arrow CAN serialize. This is the candidate replacement for the
# select(-trace) line in 01-train: the 42 traces that serialize to ~5.7GB as
# rds (captured environments) come out to ~12KB of parquet as text.
axed_str <- lgbm_search %>%
lightsnip::axe_tune_data() %>%
mutate(.notes = map(
.notes,
~ mutate(.x, trace = map_chr(trace, function(tr) {
if (is.null(tr)) NA_character_ else paste(format(tr), collapse = "\n")
}))
))axed_str %>%
tidyr::unnest(.notes) %>%
select(any_of(c("id", ".iter")), location, type, trace)axed_str_unnest$trace[1] |> cat()This is an example of what the trace looks like after formatting it in a character format, which is necessary for inspection/persistence because they are originally rds trace types of objects with super high file size |
| cat(note, "\n") | ||
| if (!is.null(trace)) cat(paste(format(trace), collapse = "\n"), "\n") | ||
| }) | ||
|
|
There was a problem hiding this comment.
Implemented Billy's idea here.
Here is an example of an output produced from this code: cloudwatch logs. Navigate to the earlier portion of the logs (CV) and you'll see it.
It shows the trace for the mse_cov induced warning message, as trace messages come up for both warnings and error messages
There was a problem hiding this comment.
Hmm, I'm torn on this approach. I think it's sensible to print the warnings, but I also worry that we'll be printing and saving a huge number of extraneous tracebacks, since a warning trace that is common to all folds/iterations/locations will get printed on every fold/iteration/location. That might actually make the logs harder to deal with, since we'd have to wade through a whole bunch of noise.
What do you two think? I see a few options:
- Go with Michael's original idea, and strip the trace entirely
- Keeps the logs as clean as possible, but makes it harder to debug warnings; we could perhaps mitigate this a bit by making it clear how to re-enable this warning logging if we need to debug in the future
- Try to deduplicate the warnings and traces even further, so that we don't print the same warning for every fold/iteration/location
-Strikes a balance between verbosity and ability to debug, but I suspect the code to do this will be convoluted and hard to understand - Only print warnings and traces for the first fold/iteration/location, assuming they will all be identical
- Might be less convoluted than 2, but I have very low confidence that the underlying assumption (all folds/iterations/locations have identical warnings) is always true, since IIRC the sample can be different
What do you two think? I don't have super strong opinions here, especially since CV is a rarely used feature, so I'm happy to move forward with whatever approach the group decides on
There was a problem hiding this comment.
I'm leaning towards option 1, here's what I'm thinking:
- I'm having a trouble seeing a situation where a CV-related error happens, and we are significantly less equipped to deal with it because we didn't persists these traces
- This data structure surrounding cv logging (for me) is complex and difficult to reason about, so I'd prefer to lean away from it instead of into it
We get a bottom line error message in cloudwatch regardless. And if that doesn't help us out enough, with some subsetting we can tease out a real stack trace locally, or even temporarily put some log printing code in the pipeline if we really need to troubleshoot
There was a problem hiding this comment.
I'm down for that, unless @wrridgeway objects! Happy to discuss this out loud today if it'd be faster.
There was a problem hiding this comment.
Woof, yeah, that output is pretty rough. I'm inclined to go with option 1 as well, I don't think these trace notes actually help seeing what they look like in the logs. So long as we get an error that points us in the right direction we can just debug this issue locally.
| notes = paste(unique(note), collapse = "; "), | ||
| .groups = "drop" | ||
| ), | ||
| by = c("id", ".iter") |
| # Should the train stage run full cross-validation? Otherwise, the model | ||
| # will be trained with the default hyperparameters specified below | ||
| cv_enable: false | ||
| cv_enable: true |
There was a problem hiding this comment.
TODO: revert to master. I used this to trim the CV runtime to test in the cloudwatch logs
wagnerlmichael
changed the title
tune bumptune bump and fix upload bug
| rename(., notes = .notes) %>% | ||
| tidyr::unnest(cols = notes) %>% | ||
| rename(notes = note) | ||
| read_parquet(paths$output$parameter_raw$local) %>% |
There was a problem hiding this comment.
Question: is there a reason we can't use . here instead of re-reading the file?
3 participants
Running into the error
I stumbled upon this bug while attempting to confirm the upload stage warning bug discovered in the condo
mse_covPR.My original objective was to reproduce this bug for the res model. First, I subsetted the CV hyperparam search and the training data to attempt to reproduce the
mse_covupload bug with less CV batch spend, but I was met with a different error - error message in cloudwatchThen, I kicked off a CV run off of master in an effort to see if this was a result of my CV shirink/subset, and the error was reproduced on main: error message in cloudwatch
Root cause
The problem is that there is a new trace column persisted in our new tune version. #533 upgraded
tunefrom 1.x to 2.1.0.tune2.x added atracecolumn to the.notestibbles inside tuning results.The column holds rlang call-stack objects, one per captured warning. At the end of CV, the train stage writes the raw tuning results to parquet:
Arrow has no serialization for a list of call stacks, so the write fails with
Cannot infer type from vector. The crash happens after tuning completes.Any warning captured during tuning triggers the bug. The
mse_covobjective guarantees warnings, becausepredict.lgb.Booster()warns once per predict call for custom objectives.Verify solution works
This run with the fix successfully passes the train stage (but fails at the upload stage because that fix is being handled here, not in this PR).
Closes #539 and #552