Added pre-commit config and ran entire code base through it

.pre-commit-config.yaml

@@ -0,0 +1,26 @@
+repos:
+  - repo: https://github.com/pre-commit/pre-commit-hooks
+    rev: v4.1.0
+    hooks:
+      - id: trailing-whitespace
+      - id: end-of-file-fixer
+  - repo: https://github.com/pre-commit/mirrors-mypy
+    rev: v0.931
+    hooks:
+      - id: mypy
+        name: mypy (no tests or bin)
+        exclude: ^(tests/|bin/)
+        args: [--strict]
+      - id: mypy
+        name: mypy (bin)
+        files: bin/
+        args: [ --scripts-are-modules,  --ignore-missing-imports ]
+  - repo: https://github.com/PyCQA/pylint
+    rev:  v2.13.9
+    hooks:
+      - id: pylint
+        args: [ --disable=all, --rcfile=.pylintrc ] # Go through this.  Maybe we want to modify this list
+  - repo: https://github.com/psf/black
+    rev: 22.6.0
+    hooks:
+      - id: black

.pylintrc

@@ -91,6 +91,7 @@ disable=raw-checker-failed,
         useless-suppression,
         deprecated-pragma,
         use-symbolic-message-instead
+        line-too-long
 
 # Enable the message, report, category or checker with the given id(s). You can
 # either give multiple identifier separated by comma (,) or put this option

README.md

@@ -3,7 +3,7 @@
 jobsub_lite is a wrapper for Condor job submission, intended
 to be backwards compatible with the actively used options of
 the past Fermilab jobsub tools, while being smaller and easier
-to maintain, and handling new requirements (i.e SciTokens 
+to maintain, and handling new requirements (i.e SciTokens
 authentication, etc.)
 
 The basic design of jobsub_lite is straightforward. It will:
@@ -35,4 +35,3 @@ This is currently implemented using Python's `requests` module for https access
 ## Tempates of .cmd .sh, and .dag files
 
 The [Jinja](http://jinja.pocoo.org/docs/dev/templates/) template code is used to generate the job submission files.  For example, the template for the .cmd file for a simple submission is [simple.cmd](https://github.com/marcmengel/jobsub_lite/blob/master/templates/simple/simple.cmd) where a name (or expression) in doubled curly braces `{{name}}` is replaced when generating the output, and conditional geneartion is done with `{%if expr%}...{%endif%}`.  (There are other Jinja features, but that is mainly what is used now in jobsub_lite).  The majority of template replacement values are directly command line options or their defaults, which makes adding new features easy; you add an option (say `--fred` ) to the command line parser, and then add a suitable entry to the appropriate template(s) using that value ( `{{fred}}` or `{%if fred %} xyzzy={{fred}} {%endif%}` .
-

bin/condor_submit

@@ -23,7 +23,6 @@ import sys
 import os
 import os.path
 import argparse
-from typing import Dict, Union
 import htcondor
 
 #
@@ -48,13 +47,7 @@ class StoreGroupinEnvironment(argparse.Action):
 
     # pylint: disable=too-few-public-methods
 
-    def __call__(
-        self,
-        parser: argparse.ArgumentParser,
-        namespace: argparse.Namespace,
-        values: Dict[str, str],
-        option_string: Union[None, str] = None,
-    ):
+    def __call__(self, parser, namespace, values, option_string):  # type: ignore
         os.environ["GROUP"] = values
         setattr(namespace, self.dest, values)
 

bin/decode_token.sh

@@ -9,8 +9,8 @@
 decode_token() {
     #
     # token is 3 base64 segments separated by dots
-    # we want the middle one. 
-    # the ending of the base64 segment is sometimes 
+    # we want the middle one.
+    # the ending of the base64 segment is sometimes
     # chopped, so ignore errors about that
     #
     sed -e 's/.*\.\(.*\)\..*/\1==/' "$1" 2>/dev/null | base64 -d  2>/dev/null
@@ -20,9 +20,9 @@ get_field() {
     #
     # filter out a field from a json block
     # rip off up to "fieldname":, then rip off everything
-    # after the comma.  
+    # after the comma.
     #
-    # BUGS 
+    # BUGS
     #    Borked for fields that contain commas.
     #
     sed -e "s/.*\"$1\"://" -e s'/,.*//'
@@ -35,7 +35,7 @@ usage() {
     echo "option -e lets you extract a particular field"
 }
 
-case x$1 in 
+case x$1 in
 x-e) extractfilt="get_field $2"; shift; shift;;
 x-h) usage; exit 0;;
 x)   usage; exit 1;;

bin/jobsub_cmd

@@ -26,7 +26,6 @@ import argparse
 import os
 import sys
 import re
-from typing import Union, Dict
 
 PREFIX = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
 sys.path.append(os.path.join(PREFIX, "lib"))
@@ -39,13 +38,7 @@ class StoreGroupinEnvironment(argparse.Action):
 
     # pylint: disable=too-few-public-methods
 
-    def __call__(
-        self,
-        parser: argparse.ArgumentParser,
-        namespace: argparse.Namespace,
-        values: Dict[str, str],
-        option_string: Union[None, str] = None,
-    ):
+    def __call__(self, parser, namespace, values, option_string):  # type: ignore
         os.environ["GROUP"] = values
         setattr(namespace, self.dest, values)
 

bin/jobsub_submit

@@ -23,7 +23,7 @@ import glob
 import os
 import os.path
 import sys
-from typing import Union, List, Dict
+from typing import Union, List, Dict, Any
 
 #
 # we are in prefix/bin/jobsub_submit, so find our prefix
@@ -62,7 +62,7 @@ def get_basefiles(dlist: List[str]) -> List[str]:
 
 
 def render_files(
-    srcdir: str, values: Dict[str, str], dest: str, dlist: Union[None, List[str]] = None
+    srcdir: str, values: Dict[str, Any], dest: str, dlist: Union[None, List[str]] = None
 ):
     """use jinja to render the templates from srcdir into the dest directory
     using values dict for substitutions
@@ -92,7 +92,7 @@ def render_files(
         except jinja.exceptions.UndefinedError as e:
             err = f"""Cannot render template file {f} due to undefined template variables.
 {e}
-Please open a ticket to the Service Desk and include this error message 
+Please open a ticket to the Service Desk and include this error message
 in its entirety.
 """
             print(err)
@@ -107,7 +107,7 @@ def cleanup(tmp: str) -> None:
     # we actually leave everything in the sandbox right now..
 
 
-def do_dataset_defaults(varg: Dict[str, str]) -> None:
+def do_dataset_defaults(varg: Dict[str, Any]) -> None:
     """
     make sure to pass appropriate SAM_* environment variables if we
     are doing datasets.  Pick a SAM_PROJECT name if we don't have one.

config.d/50-jobsub_lite.configs

@@ -8,14 +8,14 @@
 #
 SEC_CREDENTIAL_GETTOKEN_OPTS=-a fermicloud543.fnal.gov
 CREDMON_OAUTH=/usr/sbin/condor_credmon_vault
-SEC_CREDENTIAL_STORER=/usr/bin/condor_vault_storer 
+SEC_CREDENTIAL_STORER=/usr/bin/condor_vault_storer
 
 #
-# We want to use scitokens, and our service certs are signed 
+# We want to use scitokens, and our service certs are signed
 # by grid authorities
 #
 AUTH_SSL_CLIENT_CADIR=/etc/grid-security/certificates
-SEC_CLIENT_AUTHENTICATION_METHODS=SCITOKENS 
+SEC_CLIENT_AUTHENTICATION_METHODS=SCITOKENS
 
 #
 # Authentication requirement:
@@ -29,4 +29,3 @@ SEC_CLIENT_INTEGRITY=OPTIONAL
 # Not sure why we need these
 #
 LOG=/storage/local/data1/condor/log
-

lib/condor.py

@@ -20,10 +20,10 @@
 import re
 import random
 import subprocess
-from typing import Dict, List
+from typing import Dict, List, Any, Tuple, Optional, Union
 
-import htcondor
-import classad
+import htcondor  # type: ignore
+import classad  # type: ignore
 
 import packages
 
@@ -84,7 +84,7 @@ def get_schedd(vargs: Dict[str, str]) -> classad.ClassAd:
     return res
 
 
-def load_submit_file(filename: str) -> Dict[str, str]:
+def load_submit_file(filename: str) -> Tuple[Any, Optional[int]]:
     """pull in a condor submit file, make a dictionary"""
 
     #
@@ -113,7 +113,9 @@ def load_submit_file(filename: str) -> Dict[str, str]:
 
 
 # pylint: disable-next=dangerous-default-value
-def submit(f: str, vargs: Dict[str, str], schedd_name: str, cmd_args: List[str] = []):
+def submit(
+    f: str, vargs: Dict[str, str], schedd_name: str, cmd_args: List[str] = []
+) -> Union[Any, bool]:
     """Actually submit the job, using condor python bindings"""
 
     schedd_args = f"-remote {schedd_name}"
@@ -177,7 +179,7 @@ def submit(f: str, vargs: Dict[str, str], schedd_name: str, cmd_args: List[str]
 # pylint: disable-next=dangerous-default-value
 def submit_dag(
     f: str, vargs: Dict[str, str], schedd_name: str, cmd_args: List[str] = []
-):
+) -> Union[Any, bool]:
     """
     Actually submit the dag
     for the moment, we call the commandline condor_submit_dag,

lib/creds.py

@@ -15,12 +15,12 @@
 # limitations under the License.
 """ credential related routines """
 import os
-from typing import Dict
+from typing import Dict, Tuple
 
 import fake_ifdh
 
 # pylint: disable-next=dangerous-default-value
-def get_creds(args: Dict[str, str] = {}):
+def get_creds(args: Dict[str, str] = {}) -> Tuple[str, str]:
     """get credentials -- Note this does not currently push to
     myproxy, nor does it yet deal with tokens, but those should
     be done here as needed.

lib/dagnabbit.py

@@ -17,9 +17,9 @@
 import sys
 import os
 import os.path
-from typing import Dict
+from typing import Dict, List, Any
 
-import jinja2 as jinja
+import jinja2 as jinja  # type: ignore
 
 import creds
 from get_parser import get_parser
@@ -28,11 +28,11 @@
 
 def parse_dagnabbit(
     srcdir: str,
-    values: Dict[str, str],
+    values: Dict[str, Any],
     dest: str,
     schedd_name: str,
     debug_comments: bool = True,
-):
+) -> None:
     """
     parse a dagnabbit dag file generating a .dag file and .cmd files
     in the dest directory, using global cmdline options from values
@@ -51,7 +51,7 @@ def parse_dagnabbit(
         in_parallel = False
         in_serial = False
         last_serial = None
-        parallel_l = []
+        parallel_l: List[str] = []
         for line in df:
             line = line.strip()
             line = os.path.expandvars(line)

lib/fake_ifdh.py

@@ -24,7 +24,7 @@
 import os
 import time
 import argparse
-from typing import Union
+from typing import Union, Optional
 
 VAULT_HOST = "fermicloud543.fnal.gov"
 DEFAULT_ROLE = "Analysis"
@@ -35,7 +35,7 @@ def getTmp() -> str:
     return os.environ.get("TMPDIR", "/tmp")
 
 
-def getExp() -> str:
+def getExp() -> Union[str, None]:
     """return current experiment name"""
     for ev in ["GROUP", "EXPERIMENT", "SAM_EXPERIMENT"]:
         if os.environ.get(ev, None):
@@ -47,7 +47,7 @@ def getExp() -> str:
     return exp
 
 
-def getRole(role_override: Union[str, None] = None) -> str:
+def getRole(role_override: Optional[str] = None) -> str:
     """get current role"""
     if role_override:
         return role_override
@@ -58,11 +58,22 @@ def getRole(role_override: Union[str, None] = None) -> str:
 
 def checkToken(tokenfile: str) -> bool:
     """check if token is (almost) expired"""
+    if not os.path.exists(tokenfile):
+        return False
     exp_time = None
     cmd = f"decode_token.sh -e exp {tokenfile} 2>/dev/null"
     with os.popen(cmd, "r") as f:
         exp_time = f.read()
-    return exp_time and ((int(exp_time) - time.time()) > 60)
+    try:
+        return int(exp_time) - time.time() > 60
+    except ValueError as e:
+        print(
+            "decode_token.sh could not successfully extract the "
+            f"expiration time from token file {tokenfile}. Please open "
+            "a ticket to Distributed Computing Support if you need further "
+            "assistance."
+        )
+        raise
 
 
 def getToken(role: str = DEFAULT_ROLE) -> str:
@@ -71,7 +82,7 @@ def getToken(role: str = DEFAULT_ROLE) -> str:
     tmp = getTmp()
     exp = getExp()
     if exp == "samdev":
-        issuer = "fermilab"
+        issuer: Optional[str] = "fermilab"
     else:
         issuer = exp
 
@@ -112,7 +123,7 @@ def getProxy(role: str = DEFAULT_ROLE) -> str:
         igroup = exp
     else:
         issuer = "fermilab"
-        igroup = "fermilab/" + exp
+        igroup = f"fermilab/{exp}"
     vomsfile = f"{tmp}/x509up_{exp}_{role}_{pid}"
     chk_cmd = f"voms-proxy-info -exists -valid 0:10 -file {vomsfile}"
     if 0 != os.system(chk_cmd):
@@ -155,9 +166,9 @@ def cp(src: str, dest: str) -> None:
 
     try:
         if opts.command[0] == "cp":
-            commands[opts.command[0]](*opts.cpargs[0])
+            commands[opts.command[0]](*opts.cpargs[0])  # type: ignore
         else:
-            result = commands[opts.command[0]](myrole)
+            result = commands[opts.command[0]](myrole)  # type: ignore
             if result is not None:
                 print(result)
     except PermissionError as pe:

lib/get_parser.py

@@ -17,7 +17,7 @@
 # pylint: disable=too-few-public-methods
 import argparse
 import os
-from typing import Union
+from typing import Union, Any
 
 
 def verify_executable_starts_with_file_colon(s: str) -> str:
@@ -37,9 +37,9 @@ def __call__(
         self,
         parser: argparse.ArgumentParser,
         namespace: argparse.Namespace,
-        values,
+        values: Any,
         option_string: Union[None, str] = None,
-    ):
+    ) -> None:
         os.environ["GROUP"] = values
         setattr(namespace, self.dest, values)
 

lib/packages.py

@@ -41,7 +41,7 @@ def pkg_find(p: str, qual: str = "") -> None:
     if not SAVED_ENV:
         SAVED_ENV = os.environ.copy()
     path = None
-    if not path and os.environ.get("SPACK_ROOT", None):
+    if not path and os.environ.get("SPACK_ROOT"):
         cmd = f"spack find --paths --variants '{p} os=fe' 'py-{p} os=fe'"
         with os.popen(cmd, "r") as f:
             for line in f:
@@ -50,7 +50,7 @@ def pkg_find(p: str, qual: str = "") -> None:
                 path = line.split()[1]
                 break
 
-    if not path and os.environ.get("PRODUCTS", None):
+    if not path and os.environ.get("PRODUCTS"):
         cmd = f"ups list -a4 -Kproduct:@prod_dir {p} {qual}, -a0 -Kproduct:@prod_dir {p} {qual}"
         with os.popen(cmd, "r") as f:
             for line in f: