search

NordicHPC / sonar

Tool to profile usage of HPC resources by regularly probing processes using ps.
GNU General Public License v3.0
8 stars 5 forks source link

Fix #152 - Bump version, documentation, tidy up #162

Closed lars-t-hansen closed 1 month ago

lars-t-hansen commented 3 months ago

Evolving - to land after everything else has landed.

bast commented 3 months ago

After I merge and resolve the other PRs, I will fix conflict here and also update the lock file and add it to this PR.

bast commented 3 months ago

Conflict fixed and Cargo.lock updated.

bast commented 3 months ago

Before merging, let's run cargo clippy. It indicates few potential problems not caught by cargo check.

lars-t-hansen commented 3 months ago

Formatting might be in order, too.

lars-t-hansen commented 3 months ago

I'll pull it over to a machine with GPUs to make sure things look OK there too.

lars-t-hansen commented 3 months ago

I introduced a bug in command line parsing when I removed clap. Previously for sonar one could write --option=value not just --option value. The scripts use the former syntax and I think it's useful to allow that. I'll create a fix. To keep things simple, I'll attach the fix to this PR.

lars-t-hansen commented 3 months ago

That seems to work fine. I'm going to think about whether we can have some test cases to test command line parsing.

lars-t-hansen commented 2 months ago

I guess I went a little crazy with the testing. But it's time to review this probably.

bast commented 2 months ago

Are the tests run as part of the workflow/pipeline/actions?

lars-t-hansen commented 2 months ago

Are the tests run as part of the workflow/pipeline/actions?

They are not, for two reasons (that are mostly the same reason). One, I don't know how to make that happen :-) Two, one of the tests has a dependency on an external tool jq to syntax check JSON, and I don't know how to set that up.

The dependency on jq can probably be fixed, if it turns out to be a problem. It was an expedient solution. We could create simple, custom testing programs for syntax checking JSON and CSV, and use those here. Indeed the Jobanalyzer repo already has one of those that I wrote before discovering jq.

It would be great if these tests could be run as part of the workflow, though.

bast commented 2 months ago

OK I will try to add those to our workflow.

lars-t-hansen commented 2 months ago

Rebased.

bast commented 2 months ago

Working on adding the tests ...

lars-t-hansen commented 2 months ago

Working on adding the tests ...

That's great. When you get to a point where it works, also note some tests landed with #169 that should be added to the test runner.

bast commented 2 months ago

Rebasing ...

bast commented 2 months ago

The test fails now but for good reasons.

bast commented 1 month ago

Can you please verify 284d507? This restores one shell test but I want a second pair of eyes that I did not misunderstand something.

lars-t-hansen commented 1 month ago

Yes, that looks right - --bartchless and --rollup are incompatible now, so splitting the test is the best fix.

bast commented 1 month ago

Now something else fails. But it passes on my laptop. Looking into it ...

bast commented 1 month ago

Now we are all green finally. Ready to go from my perspective.

lars-t-hansen commented 1 month ago

I'll take a quick look.

lars-t-hansen commented 1 month ago

LGTM, merge at will. I don't think I had anything outstanding on this one, getting rid of subprocess would be nice but I think that's a separate Issue.