Skip to content

Commit 4635090

Browse files
mr-cmr-c
authored
Temp cidfile cleanup, start a style guide (#1005)
1 parent bfe56f3 commit 4635090

File tree

3 files changed

+83
-60
lines changed

3 files changed

+83
-60
lines changed

‎CONTRIBUTING.md

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,9 @@
1+
Style guide:
2+
PEP-8
3+
Python 2.7 + Python 3.4+ compatible code
4+
PEP-484 type hints, in comments for Python 2.7 compatability
5+
Vertically align the type hints in function definitions
6+
17
In order to contribute to the development of ``cwltool``, you need to install cwltool from source (preferably in a virtual environment):
28
Here's a rough
39
- Install virtualenv via pip: ``pip install virtualenv``
@@ -12,4 +18,4 @@ Here's a rough
1218
- If you want to run specific tests, say ``unit tests`` in Python 3.5, then: ``tox -e py35-unit``.
1319
- Look at ``tox.ini`` for all available tests and runtimes
1420
- If tests are passing, you can simply commit and create a PR on ``cwltool`` repo:
15-
- After you're done working on the ``cwltool``, you can deactivate the virtual environment: ``deactivate``
21+
- After you're done working on the ``cwltool``, you can deactivate the virtual environment: ``deactivate``

‎cwltool/docker.py

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -119,9 +119,10 @@ def get_image(docker_requirement, # type: Dict[Text, Text]
119119
del split[2]
120120

121121
# check for repository:tag match or image id match
122-
if (match and
123-
((split[0] == match.group(1) and split[1] == match.group(2)) or
124-
docker_requirement["dockerImageId"] == match.group(3))):
122+
if (match
123+
and (
124+
(split[0] == match.group(1) and split[1] == match.group(2))
125+
or docker_requirement["dockerImageId"] == match.group(3))):
125126
found = True
126127
break
127128
except ValueError:
@@ -354,7 +355,7 @@ def create_runtime(self,
354355
"please check it first")
355356
exit(2)
356357
else:
357-
cidfile_dir = os.getcwd()
358+
cidfile_dir = tempfile.mkdtemp(dir=runtimeContext.tmpdir_prefix)
358359

359360
cidfile_name = datetime.datetime.now().strftime("%Y%m%d%H%M%S-%f") + ".cid"
360361
if runtimeContext.cidfile_prefix is not None:

‎cwltool/job.py

Lines changed: 71 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -123,18 +123,18 @@ def deref_links(outputs): # type: (Any) -> None
123123
deref_links(output)
124124

125125

126-
def relink_initialworkdir(pathmapper, # type: PathMapper
127-
host_outdir, # type: Text
128-
container_outdir, # type: Text
126+
def relink_initialworkdir(pathmapper, # type: PathMapper
127+
host_outdir, # type: Text
128+
container_outdir, # type: Text
129129
inplace_update=False # type: bool
130130
): # type: (...) -> None
131131
for _, vol in pathmapper.items():
132132
if not vol.staged:
133133
continue
134134

135135
if (vol.type in ("File", "Directory") or (
136-
inplace_update and vol.type in
137-
("WritableFile", "WritableDirectory"))):
136+
inplace_update and vol.type in
137+
("WritableFile", "WritableDirectory"))):
138138
if not vol.target.startswith(container_outdir):
139139
# this is an input file written outside of the working
140140
# directory, so therefor ineligable for being an output file.
@@ -162,12 +162,12 @@ def relink_initialworkdir(pathmapper, # type: PathMapper
162162

163163
class JobBase(with_metaclass(ABCMeta, HasReqsHints)):
164164
def __init__(self,
165-
builder, # type: Builder
166-
joborder, # type: Dict[Text, Union[Dict[Text, Any], List, Text, None]]
165+
builder, # type: Builder
166+
joborder, # type: Dict[Text, Union[Dict[Text, Any], List, Text, None]]
167167
make_path_mapper, # type: Callable[..., PathMapper]
168-
requirements, # type: List[Dict[Text, Text]]
169-
hints, # type: List[Dict[Text, Text]]
170-
name, # type: Text
168+
requirements, # type: List[Dict[Text, Text]]
169+
hints, # type: List[Dict[Text, Text]]
170+
name, # type: Text
171171
): # type: (...) -> None
172172
self.builder = builder
173173
self.joborder = joborder
@@ -231,9 +231,9 @@ def _setup(self, runtimeContext): # type: (RuntimeContext) -> None
231231
for p in self.generatemapper.files()}, indent=4))
232232

233233
def _execute(self,
234-
runtime, # type: List[Text]
235-
env, # type: MutableMapping[Text, Text]
236-
runtimeContext, # type: RuntimeContext
234+
runtime, # type: List[Text]
235+
env, # type: MutableMapping[Text, Text]
236+
runtimeContext, # type: RuntimeContext
237237
monitor_function=None # type: Optional[Callable]
238238
): # type: (...) -> None
239239

@@ -341,8 +341,9 @@ def _execute(self,
341341
except Exception as e:
342342
_logger.exception(u"Exception while running job")
343343
processStatus = "permanentFail"
344-
if runtimeContext.research_obj is not None and self.prov_obj is not None and \
345-
runtimeContext.process_run_id is not None:
344+
if runtimeContext.research_obj is not None \
345+
and self.prov_obj is not None \
346+
and runtimeContext.process_run_id is not None:
346347
# creating entities for the outputs produced by each step (in the provenance document)
347348
self.prov_obj.record_process_end(str(self.name), runtimeContext.process_run_id,
348349
outputs, datetime.datetime.now())
@@ -431,16 +432,16 @@ class ContainerCommandLineJob(with_metaclass(ABCMeta, JobBase)):
431432

432433
@abstractmethod
433434
def get_from_requirements(self,
434-
r, # type: Dict[Text, Text]
435-
pull_image, # type: bool
436-
force_pull=False, # type: bool
435+
r, # type: Dict[Text, Text]
436+
pull_image, # type: bool
437+
force_pull=False, # type: bool
437438
tmp_outdir_prefix=DEFAULT_TMP_PREFIX # type: Text
438439
): # type: (...) -> Optional[Text]
439440
pass
440441

441442
@abstractmethod
442443
def create_runtime(self,
443-
env, # type: MutableMapping[Text, Text]
444+
env, # type: MutableMapping[Text, Text]
444445
runtime_context # type: RuntimeContext
445446
): # type: (...) -> Tuple[List[Text], Optional[Text]]
446447
""" Return the list of commands to run the selected container engine."""
@@ -455,39 +456,39 @@ def append_volume(runtime, source, target, writable=False):
455456

456457
@abstractmethod
457458
def add_file_or_directory_volume(self,
458-
runtime, # type: List[Text]
459-
volume, # type: MapperEnt
459+
runtime, # type: List[Text]
460+
volume, # type: MapperEnt
460461
host_outdir_tgt # type: Optional[Text]
461462
): # type: (...) -> None
462463
"""Append volume a file/dir mapping to the runtime option list."""
463464
pass
464465

465466
@abstractmethod
466467
def add_writable_file_volume(self,
467-
runtime, # type: List[Text]
468-
volume, # type: MapperEnt
468+
runtime, # type: List[Text]
469+
volume, # type: MapperEnt
469470
host_outdir_tgt, # type: Optional[Text]
470-
tmpdir_prefix # type: Text
471+
tmpdir_prefix # type: Text
471472
): # type: (...) -> None
472473
"""Append a writable file mapping to the runtime option list."""
473474
pass
474475

475476
@abstractmethod
476477
def add_writable_directory_volume(self,
477-
runtime, # type: List[Text]
478-
volume, # type: MapperEnt
478+
runtime, # type: List[Text]
479+
volume, # type: MapperEnt
479480
host_outdir_tgt, # type: Optional[Text]
480-
tmpdir_prefix # type: Text
481+
tmpdir_prefix # type: Text
481482
): # type: (...) -> None
482483
"""Append a writable directory mapping to the runtime option list."""
483484
pass
484485

485486
def create_file_and_add_volume(self,
486-
runtime, # type: List[Text]
487-
volume, # type: MapperEnt
487+
runtime, # type: List[Text]
488+
volume, # type: MapperEnt
488489
host_outdir_tgt, # type: Optional[Text]
489-
secret_store, # type: Optional[SecretStore]
490-
tmpdir_prefix # type: Text
490+
secret_store, # type: Optional[SecretStore]
491+
tmpdir_prefix # type: Text
491492
): # type: (...) -> Text
492493
"""Create the file and add a mapping."""
493494
if not host_outdir_tgt:
@@ -514,10 +515,10 @@ def create_file_and_add_volume(self,
514515
return host_outdir_tgt or new_file
515516

516517
def add_volumes(self,
517-
pathmapper, # type: PathMapper
518-
runtime, # type: List[Text]
519-
tmpdir_prefix, # type: Text
520-
secret_store=None, # type: Optional[SecretStore]
518+
pathmapper, # type: PathMapper
519+
runtime, # type: List[Text]
520+
tmpdir_prefix, # type: Text
521+
secret_store=None, # type: Optional[SecretStore]
521522
any_path_okay=False # type: bool
522523
): # type: (...) -> None
523524
"""Append volume mappings to the runtime option list."""
@@ -543,7 +544,10 @@ def add_volumes(self,
543544
self.add_writable_directory_volume(
544545
runtime, vol, host_outdir_tgt, tmpdir_prefix)
545546
elif vol.type in ["CreateFile", "CreateWritableFile"]:
546-
self.create_file_and_add_volume(runtime, vol, host_outdir_tgt, secret_store, tmpdir_prefix)
547+
new_path = self.create_file_and_add_volume(
548+
runtime, vol, host_outdir_tgt, secret_store, tmpdir_prefix)
549+
pathmapper.update(
550+
key, new_path, vol.target, vol.type, vol.staged)
547551

548552
def run(self, runtimeContext): # type: (RuntimeContext) -> None
549553
if not os.path.exists(self.tmpdir):
@@ -615,7 +619,8 @@ def run(self, runtimeContext): # type: (RuntimeContext) -> None
615619
monitor_function = None
616620
if cidfile:
617621
monitor_function = functools.partial(
618-
self.docker_monitor, cidfile, runtimeContext.tmpdir_prefix)
622+
self.docker_monitor, cidfile, runtimeContext.tmpdir_prefix,
623+
not bool(runtimeContext.cidfile_dir))
619624
self._execute(runtime, env, runtimeContext, monitor_function)
620625

621626
@staticmethod
@@ -624,7 +629,7 @@ def docker_get_memory(cid): # type: (Text) -> int
624629
try:
625630
memory = subprocess.check_output(
626631
['docker', 'inspect', '--type', 'container', '--format',
627-
'{{.HostConfig.Memory}}', cid])
632+
'{{.HostConfig.Memory}}', cid], stderr=subprocess.DEVNULL) # type: ignore
628633
except subprocess.CalledProcessError:
629634
pass
630635
if memory:
@@ -633,11 +638,21 @@ def docker_get_memory(cid): # type: (Text) -> int
633638
return value
634639
return psutil.virtual_memory().total
635640

636-
def docker_monitor(self, cidfile, tmpdir_prefix, process):
637-
# type: (Text, Text, subprocess.Popen) -> None
641+
def docker_monitor(self, cidfile, tmpdir_prefix, cleanup_cidfile, process):
642+
# type: (Text, Text, bool, subprocess.Popen) -> None
643+
"""Record memory usage of the running Docker container."""
644+
# Todo: consider switching to `docker create` / `docker start`
645+
# instead of `docker run` as `docker create` outputs the container ID
646+
# to stdout, but the container is frozen, thus allowing us to start the
647+
# monitoring process without dealing with the cidfile or too-fast
648+
# container execution
638649
cid = None
639650
while cid is None:
640651
time.sleep(1)
652+
if process.returncode is not None:
653+
if cleanup_cidfile:
654+
os.remove(cidfile)
655+
return
641656
try:
642657
with open(cidfile) as cidhandle:
643658
cid = cidhandle.readline().strip()
@@ -663,21 +678,22 @@ def docker_monitor(self, cidfile, tmpdir_prefix, process):
663678
break
664679
_logger.info(u"[job %s] Max memory used: %iMiB", self.name,
665680
int((max_mem_percent * max_mem) / (2 ** 20)))
666-
667-
668-
def _job_popen(
669-
commands, # type: List[Text]
670-
stdin_path, # type: Optional[Text]
671-
stdout_path, # type: Optional[Text]
672-
stderr_path, # type: Optional[Text]
673-
env, # type: MutableMapping[AnyStr, AnyStr]
674-
cwd, # type: Text
675-
job_dir, # type: Text
676-
job_script_contents=None, # type: Text
677-
timelimit=None, # type: int
678-
name=None, # type: Text
679-
monitor_function=None # type: Optional[Callable]
680-
): # type: (...) -> int
681+
if cleanup_cidfile:
682+
os.remove(cidfile)
683+
684+
685+
def _job_popen(commands, # type: List[Text]
686+
stdin_path, # type: Optional[Text]
687+
stdout_path, # type: Optional[Text]
688+
stderr_path, # type: Optional[Text]
689+
env, # type: MutableMapping[AnyStr, AnyStr]
690+
cwd, # type: Text
691+
job_dir, # type: Text
692+
job_script_contents=None, # type: Text
693+
timelimit=None, # type: int
694+
name=None, # type: Text
695+
monitor_function=None # type: Optional[Callable]
696+
): # type: (...) -> int
681697

682698
if job_script_contents is None and not FORCE_SHELLED_POPEN:
683699

0 commit comments

Comments
 (0)