Conductor process logging - #475
Conversation
…t was previously stderr/stdout only
doutriaux1
left a comment
doutriaux1
left a comment
There was a problem hiding this comment.
Looks good I have a few comments though.
| @classmethod | ||
| def _conductor_record_path(cls, output_path, conductor_id): | ||
| registry_path = cls._conductor_registry_path(output_path) | ||
| return make_safe_path(registry_path, "{}.json".format(conductor_id)) |
| assert conductor_id in conductors | ||
| record = conductors[conductor_id] | ||
| assert record["conductor_id"] == conductor_id | ||
| assert record["study_name"] == "dummy_study" |
There was a problem hiding this comment.
we probably need a study_id as well no? If we ever decide to put everything in a single db. I do realize at the moment it's not necessary since there's only study and one conductor
There was a problem hiding this comment.
conductor_step to show which step it is at (at least until we can link to a study record which will link to each expanded steps' records.
There was a problem hiding this comment.
do we want the things like throttle sleeptime etc
There was a problem hiding this comment.
it might be in argv but would be nice to get them in a dedicated place
There was a problem hiding this comment.
looks like you're not tesing everything here, e.g. python_executable
| ROOTLOGGER.addHandler(handler) | ||
| ROOTLOGGER.setLevel(loglevel) | ||
|
|
||
| LOGGER.info("Running Maestro Conductor standalone.") |
There was a problem hiding this comment.
is there a non standalone mode?
|
|
||
| @staticmethod | ||
| def _get_host_addresses(): | ||
| """Return best-effort IP addresses for the current host.""" |
There was a problem hiding this comment.
can a host have mutliple ips?
There was a problem hiding this comment.
unfortunately, yes; getaddrinfo returns address for each network adapter and protocol (tcp/udp/...) on the host. tested a few clusters and only seen single addresses so far, so hopefully prioritizing the first tcp one is going to work?
| return json.load(data) or {} | ||
| except (OSError, ValueError) as exc: | ||
| last_error = exc | ||
| if attempt + 1 < attempts: |
There was a problem hiding this comment.
if attempt <= attempts ?
| return record | ||
|
|
||
| @classmethod | ||
| def get_conductors(cls, output_path): |
There was a problem hiding this comment.
there can be multiple conductors for the same study?
There was a problem hiding this comment.
not yet; first instance of this will be resuming studies after conductor shuts down to rerun steps/override restart limit/...
|
|
||
| def _new_conductor_id(self): | ||
| hostname = socket.gethostname() | ||
| return "{}.{}.{}".format(hostname, os.getpid(), uuid.uuid4().hex[:12]) |
There was a problem hiding this comment.
is it just for readability? we don't really need hostname and pid?
| self._conductor_id = self._new_conductor_id() | ||
|
|
||
| now = self._timestamp() | ||
| record = { |
There was a problem hiding this comment.
did you check these match the other part of the code that tests this ? (I didn't do it carefully)
2 participants
Adds logging of conductor processes to help track across multi-node clusters
Adds staged logging of
maestro runto capture existing stdout/stderr log messages to real study log file unless launch is aborted.