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

use logs as part of the validation #1198

Merged
merged 8 commits into from
Sep 14, 2023

Conversation

tomeichlersmith
Copy link
Member

I am updating ldmx-sw, here are the details.

What are the issues that this addresses?

This resolves #1192 by updating the CI scripts to store and compare the terminal output when running fire.

Check List

  • I successfully compiled ldmx-sw with my developments
  • I ran my developments and the following shows that they are successful.
  • NA I attached any sub-module related changes to this PR.

@tomeichlersmith tomeichlersmith linked an issue Sep 12, 2023 that may be closed by this pull request
Copy link
Member

@tvami tvami left a comment

Choose a reason for hiding this comment

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

Looks great, thanks Tom!

the timestamps printed out by some processors prevents a full text diff from being successful so we just write the diff to the output and continue no matter what
@bryngemark
Copy link
Contributor

Just noticed that the logs for, was it inclusive and maybe pileup/hcal/more? spit out a configuration error where some set parameter is wrongly typed as float or int (don’t remember which way). A good time to fix this?

@EinarElen
Copy link
Contributor

Ive been wondering what that warning comes from, if it is a parameter that is wrong wouldn't the whole thing halt? So I would have thought that it comes from somewhere else

@tomeichlersmith
Copy link
Member Author

I first noticed that warning/error when updating to the newer compiler. I am unsure where it is originating - if its in the python or C++ side of configuration. I agree with Einar, that it would halt if the parameter was wrong, but we could easily be misinterpreting some of the Python C-API functions that we are using.

For now, I think it is best to leave it since I do not know what kind of rabbit-hole tracing it will lead to. If I have time, I can open an issue with a first look at it.

@tomeichlersmith tomeichlersmith marked this pull request as draft September 13, 2023 12:44
@tomeichlersmith tomeichlersmith marked this pull request as ready for review September 13, 2023 12:45
@tomeichlersmith tomeichlersmith added this to the v3.3.0 milestone Sep 13, 2023
Since `set -e` is at the top of the script, we need to explicitly avoid any non-zero return code by using `|| true`.
@tomeichlersmith tomeichlersmith marked this pull request as draft September 13, 2023 17:11
@tomeichlersmith tomeichlersmith marked this pull request as ready for review September 13, 2023 17:11
@tomeichlersmith tomeichlersmith marked this pull request as draft September 13, 2023 19:27
@tomeichlersmith tomeichlersmith marked this pull request as ready for review September 13, 2023 19:27
@tvami
Copy link
Member

tvami commented Sep 14, 2023

I made #1199 as the possible source for the TypeError: 'float' object cannot be interpreted as an integer. Although maybe it's unrelated...

@tomeichlersmith
Copy link
Member Author

Alright, I generated the gold logs with an intentionally slightly old commit of ldmx-sw which produced slightly different logs. This means we get the following report from GitHub.

warning-eg

I don't think this is enough since the checks are still labeled as "passing" here, so I am going to move the character-count checking from warning to error and put it in a different job step.

@tomeichlersmith tomeichlersmith marked this pull request as draft September 14, 2023 13:12
@tomeichlersmith tomeichlersmith marked this pull request as ready for review September 14, 2023 13:12
@tomeichlersmith
Copy link
Member Author

Alright, the new infrastructure is now failing at the log check (as intended). The next step is to merge this and make a new patch release to make sure that I can replace the logs with newly-generated ones.

log-comp-fail

@tomeichlersmith tomeichlersmith merged commit ec628b5 into trunk Sep 14, 2023
1 check passed
@tomeichlersmith tomeichlersmith deleted the 1192-use-logs-as-part-of-the-validation branch September 14, 2023 15:39
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.

Use logs as part of the validation
4 participants