search

pitt-crc / wrappers

User focused command line wrappers around Slurm
https://crc-pages.pitt.edu/wrappers/
GNU General Public License v3.0
1 stars 1 forks source link

HH:MM:SS time formatting bug in crc-interactive #214

Closed yassinkhalifa closed 1 year ago

yassinkhalifa commented 1 year ago

It seems when entering the decimal value for the parameter "-t" or "--time" in crc-interactive it's formatted in the srun_dict as "{}:00" which makes it interpreted as "MM:SS" but if using the string format "01:00" and it's formatted in the srun_dict as "{}:00" then it's interpreted as "HH:MM:SS" which is the necessary format for "srun". I added a possible fix, please take a look and feel free to discard it and fix it in any other way because I am sure this is not the best way to fix it, I was just trying to expose where the error comes from.

yassinkhalifa commented 1 year ago

I totally agree that users don't really need the seconds part in time. If you want, I can just change that back to how it was before and just leave the decimal formatting line that I added (just modify it to be HH:MM which will be amended with seconds in srun_dict).

djperrefort commented 1 year ago

Sounds good to me.

yassinkhalifa commented 1 year ago

That's done now!

yassinkhalifa commented 1 year ago

Yeah, this is much better! I can work on that.

yassinkhalifa commented 1 year ago

I added the suggested function parse_time and made it only return component #0 because we are always interested in the hours component (the first element) which simplified the check_time part as you indicated and changed the formatting of default_time to just a decimal '1' so that it aligns with the output of parse_time. Please take a look and let me know if I misinterpreted any of your instructions.

yassinkhalifa commented 1 year ago

I went through all the requested changes and completed them. Please let me know if there is anything else I need to change.

djperrefort commented 1 year ago

@yassinkhalifa please merge this in when you are ready