Version 1.2.0: Sponge Gourd

[andtsa]

Version 1.2.0: Sponge Gourd

tl;dr:

• gourd analyse redone, now supports:
  • CSV
  • custom columns
  • custom metrics
  • grouping
• gourd.toml changes:
  • ...

breaking changes:

• gourd analyse behaves very differently, check updated CLI documentation
• ...

maintainer changes:

• can now create new metrics using ColumnGenerator(s)
• new integration test framework

qol changes:

• pipeline artefacts expire every week

still todo:

• [ ] fix divide by zero error in default metrics accumulator
• [ ] write new gourd analyse user-end documentation
• [ ] write col gen maintainer docs
• [ ] write config() integration test maintainer docs
• [ ] create issues for additional integration test scenarios desired (multi-step pipelines, more extensive gourd analyse testing, etc)
• [ ] update version in code & docs
• [ ] ...

[andtsa]

[andtsa]

[lchladek]

Usability review part 1:

The table implementation is very elegant and looks great! The default settings should fit in 80x24 though, or at least something smaller than 1048576x524288.

This should fit much better if we use relative paths, and unwrap optionals to not use Some.

I think trying to optimize gourd analyse table -s --format program,file,args,wall-time is a good start.

[andtsa]

I think trying to optimize gourd analyse table -s --format program,file,args,wall-time is a good start.

could you elaborate on this? im not sure I follow what you're asking

[lchladek]

Usability review part 2:

• change '--save' to '-o/--out'
• don't use humantime for timings, it's not readable, just make it seconds
• is it possible to sum to an integer for things like swap count?

[lchladek]

I think trying to optimize gourd analyse table -s --format program,file,args,wall-time is a good start.

could you elaborate on this? im not sure I follow what you're asking

Basically, cutting down the character count on things like "slurm" or "max-rss" is not really important. I would suggest, though, making the command above the 'default' column layout and really trying to get (those particular columns) to fit in a small window!

[andtsa]

• change '--save' to '-o/--out'

changed to --output, I kept the thing where its in 3 places in def.rs from lack of a better alternative through clap. if you or @mgazeel knows a better way to work around clap it would be much appreciated.

• don't use humantime for timings, it's not readable, just make it seconds

mm sure it is better for anything that runs in ms but for longer runs it will mean nothing if one thing finishes in 442312 seconds and another in 298452. I feel human time is better universally, but im willing to discuss.

• is it possible to sum to an integer for things like swap count?

yes but for things that are 0 or 1 very often the average will be "0" without actually being 0. it's just more informative to keep 2 decimals

[lchladek]

• change '--save' to '-o/--out'

changed to --output, I kept the thing where its in 3 places in def.rs from lack of a better alternative through clap. if you or @mgazeel knows a better way to work around clap it would be much appreciated.

Good

• don't use humantime for timings, it's not readable, just make it seconds

mm sure it is better for anything that runs in ms but for longer runs it will mean nothing if one thing finishes in 442312 seconds and another in 298452. I feel human time is better universally, but im willing to discuss.

A table should convey a good summary of the data; the example you gave actually makes it super clear that the second was 3/4 as fast. To me much better than 5d2h4m52s193ms, and additionally can be parsed by computer

• is it possible to sum to an integer for things like swap count?

yes but for things that are 0 or 1 very often the average will be "0" without actually being 0. it's just more informative to keep 2 decimals

Not sure I follow?

[lchladek]

Final request: please make a unix-style version of table for script mode. No headers, no gimmicks, just tab-separated values so that gourd analyse table -s -f name,wall_time | cut -f 2 | sort gives me the sorted wall time :)

[lchladek]

Just think a function ‘write_csv’ that takes care of the file finding, row writing, flushing, etc. would be cleaner.

On 24 Sep 2024, at 13:50, Andreas Tsatsanis @.***> wrote:

@andtsa commented on this pull request.

In src/gourd/cli/process.rs https://github.com/ConSol-Lab/gourd/pull/19#discussion_r1773785036:

                 }

• _ => bailc!("Unsupported output format {}", &output;
• "Use 'csv', 'plot-png', or 'plot-svg'.", ; "" ,),
• writer.flush()?; its far from the only non-cli logic in this file, and its 3 lines of code that you'd want somewhere else. im not sure I agree

— Reply to this email directly, view it on GitHub https://github.com/ConSol-Lab/gourd/pull/19#discussion_r1773785036, or unsubscribe https://github.com/notifications/unsubscribe-auth/A6FYVZC6NFPHOYKHUVYHB3DZYGQ7ZAVCNFSM6AAAAABNDQDBZKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMRVHE3TONJQG4. You are receiving this because you commented.

[andtsa]

Final request: please make a unix-style version of table for script mode. No headers, no gimmicks, just tab-separated values so that gourd analyse table -s -f name,wall_time | cut -f 2 | sort gives me the sorted wall time :)

done, please check

[andtsa]

• is it possible to sum to an integer for things like swap count?

yes but for things that are 0 or 1 very often the average will be "0" without actually being 0. it's just more informative to keep 2 decimals

Not sure I follow?

I noticed for some statistics the values are often 0 or 1, in which case an "average" of 0 or an "average" of 1 does not mean anything. decimals won't hurt anyone and they're useful extra information, I explicitly added them after noticing that integer rounding led to uninformative results

[lchladek]

My bad! Somehow I mistook it for a ‘grand total’. Yes, decimals are necessary.

On 24 Sep 2024, at 15:04, Andreas Tsatsanis @.***> wrote:

is it possible to sum to an integer for things like swap count? yes but for things that are 0 or 1 very often the average will be "0" without actually being 0. it's just more informative to keep 2 decimals

Not sure I follow?

I noticed for some statistics the values are often 0 or 1, in which case an "average" of 0 or an "average" of 1 does not mean anything. decimals won't hurt anyone and they're useful extra information, I explicitly added them after noticing that integer rounding led to uninformative results

— Reply to this email directly, view it on GitHub https://github.com/ConSol-Lab/gourd/pull/19#issuecomment-2372092482, or unsubscribe https://github.com/notifications/unsubscribe-auth/A6FYVZCLPQAA6LUSFYFSYODZYGZSJAVCNFSM6AAAAABNDQDBZKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNZSGA4TENBYGI. You are receiving this because you commented.

[lchladek]

This works well for my use-case! The header is still there though :)

On 24 Sep 2024, at 15:02, Andreas Tsatsanis @.***> wrote:

Final request: please make a unix-style version of table for script mode. No headers, no gimmicks, just tab-separated values so that gourd analyse table -s -f name,wall_time | cut -f 2 | sort gives me the sorted wall time :)

done, please check

— Reply to this email directly, view it on GitHub https://github.com/ConSol-Lab/gourd/pull/19#issuecomment-2372085476, or unsubscribe https://github.com/notifications/unsubscribe-auth/A6FYVZC3BLC2WNUY2POLCJDZYGZLFAVCNFSM6AAAAABNDQDBZKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNZSGA4DKNBXGY. You are receiving this because you commented.

[andtsa]

This works well for my use-case! The header is still there though :)

I think I will keep it as it is customary CSV, unless you strongly feel otherwise

[andtsa]

This works well for my use-case! The header is still there though :)

I think I will keep it as it is customary CSV, unless you strongly feel otherwise

although I realise you probably meant the footer, which I did just remove so no worries

[lchladek]

I think no header is better (if the fields are manually specified), but don’t mind. Footer does break things, however, and there are some trailing newlines for no apparent reason

On 24 Sep 2024, at 15:07, Andreas Tsatsanis @.***> wrote:

This works well for my use-case! The header is still there though :)

I think I will keep it as it is customary CSV, unless you strongly feel otherwise

— Reply to this email directly, view it on GitHub https://github.com/ConSol-Lab/gourd/pull/19#issuecomment-2372103864, or unsubscribe https://github.com/notifications/unsubscribe-auth/A6FYVZGJKSORX3SHLJ6GG3DZYGZ5RAVCNFSM6AAAAABNDQDBZKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNZSGEYDGOBWGQ. You are receiving this because you commented.

[andtsa]

I think no header is better (if the fields are manually specified), but don’t mind.

So no header iff format specified? This seems useful but not very intuitive or consistent, I'm willing to discuss since it's likely to only be used by more advanced users

Footer does break things, however, and there are some trailing newlines for no apparent reason

Can you check if this is still the case now that the footer is gone?

[lchladek]

Yes, still there unfortunately.



So no header iff format specified? This seems useful but not very intuitive or consistent, I'm willing to discuss since it's likely to only be used by more advanced users

Possibly (well, this would only apply to script mode anyway). I agree that the logic is unnecessarily complex. Either keep the headers in or out, and I understand the case for ‘in’.

On 24 Sep 2024, at 15:22, Andreas Tsatsanis @.***> wrote:

I think no header is better (if the fields are manually specified), but don’t mind.

So no header iff format specified? This seems useful but not very intuitive or consistent, I'm willing to discuss since it's likely to only be used by more advanced users

Footer does break things, however, and there are some trailing newlines for no apparent reason

Can you check if this is still the case now that the footer is gone?

— Reply to this email directly, view it on GitHub https://github.com/ConSol-Lab/gourd/pull/19#issuecomment-2372156393, or unsubscribe https://github.com/notifications/unsubscribe-auth/A6FYVZA2DOJCGMEDHMD3FMLZYG3VRAVCNFSM6AAAAABNDQDBZKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNZSGE2TMMZZGM. You are receiving this because you commented.

[andtsa]

I think no header is better (if the fields are manually specified), but don’t mind. Footer does break things, however, and there are some trailing newlines for no apparent reason

I will remove the header in all cases of script mode under the argument that I don't think I'll be doing any script users a favour by keeping it