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

Sanitization of job name and tests for PBS #53

Merged
merged 8 commits into from
Jan 20, 2025

Conversation

gpetretto
Copy link
Contributor

To address this issue Matgenix/jobflow-remote#112 introduce a sanitization for the name of the jobs for PBS and SGE. The choice of the characters to replace is based on explicit tests in the PBS and SGE containers from jobflow-remote's integration tests.

Also adding test for PBS as some regression was introduced when adding SGE. It would be better to add integration tests similar to jobflow-remote to improve the reliability.

@gpetretto
Copy link
Contributor Author

A few notes:

  • The separator for the job ids in the list of jobs command works differently for PBS and SGE. A comma for SGE and a space for PBS. Tests will need to be adapted. Waiting for updates from fix parse_jobs_list_output parsing issues with SGE #52
  • It seems that in OpenPBS the -f format option is ignored when filtering by username with the -u option:
    $ qstat -u jobflow -f
    
    4250a605a812:
                                                                Req'd  Req'd   Elap
    Job ID          Username Queue    Jobname    SessID NDS TSK Memory Time  S Time
    --------------- -------- -------- ---------- ------ --- --- ------ ----- - -----
    2003.4250a605a* jobflow  workq    myscript      958   1   1    --  01:00 R 00:00
    
    Does this work for PBSpro or other PBS versions? Should we cover this case for OpenPBS?

@gpetretto
Copy link
Contributor Author

Few updates following the merge of #52.

Concerning the query based on users, it seems that qstat shows the same format when filter by user and job_id if adding the -w. I tested this in the OpenPBS based container and it seems to work correctly.

Copy link
Member

@davidwaroquiers davidwaroquiers left a comment

Choose a reason for hiding this comment

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

Almost all good to me. I think there is one small bug in the data_objects (see comment). Then a few comments here and there. Otherwise great work!

src/qtoolkit/core/data_objects.py Outdated Show resolved Hide resolved
src/qtoolkit/io/base.py Show resolved Hide resolved
src/qtoolkit/io/pbs.py Outdated Show resolved Hide resolved
src/qtoolkit/io/pbs_base.py Outdated Show resolved Hide resolved
src/qtoolkit/io/sge.py Show resolved Hide resolved
src/qtoolkit/io/shell.py Outdated Show resolved Hide resolved
tests/io/test_shell.py Outdated Show resolved Hide resolved
@gpetretto gpetretto merged commit b440e7d into Matgenix:develop Jan 20, 2025
5 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