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

For REMIND AMTs: execute make test-full-slurm #92

Merged
merged 3 commits into from
Mar 1, 2024

Conversation

dklein-pik
Copy link
Member

Together with the REMIND AMTs execute make test-full-slurm and evaluate the result and append it to the mattermost bot message.

@dklein-pik dklein-pik requested a review from orichters March 1, 2024 16:19
Copy link
Contributor

@orichters orichters left a comment

Choose a reason for hiding this comment

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

Thanks a lot, some comments anyway.

system(paste0(gitPath, " reset --hard origin/develop && ", gitPath, " pull"))
})
# 2. rename old test-full.log
system("mv test-full.log test-full-previous.log")
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't want to keep the logs long-term? I mean, you could also adjust make test-full-slurm to something like

sbatch --qos=priority --wrap="make test-full" --job-name=test-full --mail-type=END --output=test-full-2024-03-01.log --comment="test-full-2024-03-01.log"

in REMIND's makefile (not exactly sure how this would be implemented that the date is always adjusted). Or, at the end of the script, when everything is done, you copy the test-full.log to test-full-2024-03-01.txt and avoid like that that it is overridden.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to avoid another file piling up in some folder that nobody needs. But if you really think we need the output of tests older than two weeks, I can change that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you just move it into tests subdirectory where it bothers no-one but is available if needed?

# start make test-full
# 1. pull changes to magpie develop
withr::with_dir("magpie", {
system(paste0(gitPath, " reset --hard origin/develop && ", gitPath, " pull"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
system(paste0(gitPath, " reset --hard origin/develop && ", gitPath, " pull"))
system(paste0(gitPath, " reset --hard origin/master && ", gitPath, " pull"))

Do we really want to test against MAgPIE develop? In case there is some temporaty error in MAgPIE, why should we have REMIND fails? Not sure about that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, it is a bit of a risk. However, the advantage is that it allows us to detect changes in the coupling interface immediately (= when the name of variables change that are required for the coupling) and not after it has already been established.

Copy link
Contributor

@orichters orichters Mar 4, 2024

Choose a reason for hiding this comment

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

I agree, using develop is actually the better choice.

} else if (any(grep("FAIL 1", logStatus))) {
testthatResult <- "Some tests FAIL in `make test-full`. Check test-full.log"
} else {
# "make test-full reports all tests PASS"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer something like tail(grep("[ FAIL", logStatus, value = TRUE, fixed = TRUE)) to be appended to the log status, so you see directly how many fails exists etc.

And: Why grep("Fail 1", logStatus)? Does that work if you have more than one fail?

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. I tested this with the output of make test-coupled-slurm (test-coupled.log), but its output seems to be different in terms of structure from the output of make test-full-slurm. I`ll adapt it.

@dklein-pik dklein-pik merged commit 7479bd8 into pik-piam:master Mar 1, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants