Use sanitised variable name in SGE scheduler job title

[mjclarke94]

The SGE scheduler creates a sanitised job_title variable, but doesn't actually use it.

[sphuber]

Thanks @mjclarke94 . That indeed seems unintended, however, I also wonder whether this entire cleaning block is even needed? The block seems to have been copied verbatim from the PbsBaseClass scheduler implementation for PBS based schedulers (as indicated by the reference to qsub in the comment). Is this cleaning even required for SGE? Does it have the same limitations on the job title as PBS has?

[mjclarke94]

Yeah, seems to be the same restrictions which explains the code reuse.
From the man page of qsub on an HPC using SGE:

 -N name
              Available for qsub, qsh, qrsh, qlogin and qalter only.

              The name of the job. The name should follow the "name" definition in sge_types(5).  Invalid job names will be denied at submit time.

man sge_types

name
       A name is an arbitrary string of ASCII printing characters, but may not contain  "/", ":", "@", "\", "*",  or "?".

Edit: Just reread your comment. To clarify, qsub is also the submission command for SGE.

[sphuber]

Thanks for the update @mjclarke94 . Then it seems all good to merge. It would just be good to add a test. I quickly wrote one that you can add at the end of the file tests/schedulers/test_sge.py

def test_job_name_cleaning(self):
    """Test that invalid characters are cleaned from job name."""
    from aiida.schedulers.datastructures import JobTemplate

    scheduler = SgeScheduler()

    job_tmpl = JobTemplate()
    job_tmpl.job_resource = scheduler.create_job_resource(parallel_env='mpi8', tot_num_mpiprocs=16)
    job_tmpl.job_name = 'Some/job:name@with*invalid-characters.'

    header = scheduler._get_submit_script_header(job_tmpl)
    self.assertTrue('#$ -N Somejobnamewithinvalid-characters.' in header, header)

Feel free to modify it of course if you think you can improve it. If you add this, we can run the tests and merge

[mjclarke94]

Looks good to me! Test added 🙂

[codecov]

Codecov Report

Merging #4994 (174d221) into develop (97a7fa9) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #4994   +/-   ##
========================================
  Coverage    80.13%   80.13%           
========================================
  Files          515      515           
  Lines        36692    36692           
========================================
  Hits         29398    29398           
  Misses        7294     7294           

Flag	Coverage Δ
django	74.60% <100.00%> (+0.01%)	⬆️
sqlalchemy	73.50% <100.00%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida/schedulers/plugins/sge.py	70.30% <100.00%> (+0.42%)	⬆️
aiida/transports/plugins/local.py	81.41% <0.00%> (-0.25%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97a7fa9...174d221. Read the comment docs.

[mjclarke94]

Oop. Caught by the pre-commit. Struggling to get it to install locally for some reason but I think that change should sort it.

[sphuber]

Thanks @mjclarke94