Skip to content

Fix unhelpful error messages in aggregate jobs. #825

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.

Sign up for GitHub

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

Merged
grahamc merged 2 commits into from
Oct 24, 2021
Merged

Fix unhelpful error messages in aggregate jobs. #825

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
68e689c
hydra-eval-jobs: Identify unexpected errors in handling aggregate jobs
samueldr Oct 25, 2020
b5140c1
hydra-eval-jobs: Transmit original Nix error when handling aggregate …
samueldr Oct 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 52 additions & 32 deletions src/hydra-eval-jobs/hydra-eval-jobs.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,45 +442,65 @@ int main(int argc, char * * argv)
for (auto i = state->jobs.begin(); i != state->jobs.end(); ++i) {
auto jobName = i.key();
auto & job = i.value();
// For the error message
std::string lastTriedJobName = i.key();
Copy link
Member Author

@samueldr samueldr Oct 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the initialization here is not great...

I think when I wrote that up originally I was thinking about using auto to intrinsically get the type. In that case auto lastTriedJobName = jobName is probably better.

Otherwise, initializing with "".

What would be the better option?

Copy link
Member

@andir andir Oct 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC the nlohman value key() function returns reference. auto should then deduct the type T& which is probably not what we want here. The assignments in line 454 and 466 would then override the value in the JSON document. That being said my type deduction knowledge might be a bit rusty... I would stick with the string there.


auto named = job.find("namedConstituents");
if (named == job.end()) continue;

if (myArgs.dryRun) {
for (std::string jobName2 : *named) {
auto job2 = state->jobs.find(jobName2);
if (job2 == state->jobs.end())
throw Error("aggregate job '%s' references non-existent job '%s'", jobName, jobName2);
std::string drvPath2 = (*job2)["drvPath"];
job["constituents"].push_back(drvPath2);
}
} else {
auto drvPath = store->parseStorePath((std::string) job["drvPath"]);
auto drv = store->readDerivation(drvPath);

for (std::string jobName2 : *named) {
auto job2 = state->jobs.find(jobName2);
if (job2 == state->jobs.end())
throw Error("aggregate job '%s' references non-existent job '%s'", jobName, jobName2);
auto drvPath2 = store->parseStorePath((std::string) (*job2)["drvPath"]);
auto drv2 = store->readDerivation(drvPath2);
job["constituents"].push_back(store->printStorePath(drvPath2));
drv.inputDrvs[drvPath2] = {drv2.outputs.begin()->first};
}
try {
if (myArgs.dryRun) {
for (std::string jobName2 : *named) {
lastTriedJobName = jobName2;
auto job2 = state->jobs.find(jobName2);
if (job2 == state->jobs.end())
throw Error("aggregate job '%s' references non-existent job '%s'", jobName, jobName2);
std::string drvPath2 = (*job2)["drvPath"];
job["constituents"].push_back(drvPath2);
}
} else {
auto drvPath = store->parseStorePath((std::string) job["drvPath"]);
auto drv = store->readDerivation(drvPath);

for (std::string jobName2 : *named) {
lastTriedJobName = jobName2;
auto job2 = state->jobs.find(jobName2);
if (job2 == state->jobs.end())
throw Error("aggregate job '%s' references non-existent job '%s'", jobName, jobName2);

if ((*job2).find("error") != (*job2).end()) {
Copy link
Member Author

@samueldr samueldr Oct 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needing to dereference here tripped me up. I get that this comes from auto job2 being the result of the search. Is there a pattern we could be using so that we don't have to dereference all the time?

Copy link
Member

@andir andir Oct 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

auto& something = *job2 should do the trick here and then using something instead of *job2.

Copy link
Contributor

@lukegb lukegb Aug 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, this is probably fine? I'd probably replace the (*job2). with just job2-> but other than that this seems... fine.

if (job.find("error") == job.end()) {
job["error"] = fmt("Errors aggregating aggregate job '%1%'.\n", jobName);
}
Comment on lines +472 to +474
Copy link
Member Author

@samueldr samueldr Oct 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better pattern to initialize the error message on the JSON object?

job["error"] = fmt("While handling '%1%': %2%\n", jobName2, (std::string) (*job2)["error"]);
Copy link
Member

@ncfavier ncfavier Aug 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this always overwrite the assignment on line 473?

} else {
auto drvPath2 = store->parseStorePath((std::string) (*job2)["drvPath"]);
auto drv2 = store->readDerivation(drvPath2);
job["constituents"].push_back(store->printStorePath(drvPath2));
drv.inputDrvs[drvPath2] = {drv2.outputs.begin()->first};
}
}

std::string drvName(drvPath.name());
assert(hasSuffix(drvName, drvExtension));
drvName.resize(drvName.size() - drvExtension.size());
auto h = std::get<Hash>(hashDerivationModulo(*store, drv, true));
auto outPath = store->makeOutputPath("out", h, drvName);
drv.env["out"] = store->printStorePath(outPath);
drv.outputs.insert_or_assign("out", DerivationOutput { .output = DerivationOutputInputAddressed { .path = outPath } });
auto newDrvPath = store->printStorePath(writeDerivation(*store, drv));
std::string drvName(drvPath.name());
assert(hasSuffix(drvName, drvExtension));
drvName.resize(drvName.size() - drvExtension.size());
auto h = std::get<Hash>(hashDerivationModulo(*store, drv, true));
auto outPath = store->makeOutputPath("out", h, drvName);
drv.env["out"] = store->printStorePath(outPath);
drv.outputs.insert_or_assign("out", DerivationOutput { .output = DerivationOutputInputAddressed { .path = outPath } });
auto newDrvPath = store->printStorePath(writeDerivation(*store, drv));

debug("rewrote aggregate derivation %s -> %s", store->printStorePath(drvPath), newDrvPath);
debug("rewrote aggregate derivation %s -> %s", store->printStorePath(drvPath), newDrvPath);

job["drvPath"] = newDrvPath;
job["outputs"]["out"] = store->printStorePath(outPath);
}
} catch (std::exception & e) {
// Print more information to help debugging.
printError("Unexpected error in hydra-eval-jobs when handling job '%s', when producing aggregate job '%s':", lastTriedJobName, jobName);

job["drvPath"] = newDrvPath;
job["outputs"]["out"] = store->printStorePath(outPath);
// And throw the original exception!
throw;
}

job.erase("namedConstituents");
Expand Down