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

Improve submit_batch_script logging #219

Merged
merged 1 commit into from
Mar 16, 2024
Merged

Conversation

cmd-ntrf
Copy link
Contributor

@cmd-ntrf cmd-ntrf commented Jun 4, 2021

Adds:

  • logging of subvars (info)
  • logging of self.get_env (debug)

Changes:

  • the logging level of the submit script from info to debug.

The rational is that it is easier to parse subvars dictionary in a separate log than the multiline submit script, which would then be only useful in a debug context.

Removes:

  • cmd from the Job submitted info log, since it is was already logged before.

This PR also introduces the usage of logging argument instead of concatenating string with +.
I will follow with a second PR that standardize logging to use argument instead of string concatenation systematically.

@cmd-ntrf cmd-ntrf force-pushed the log branch 2 times, most recently from 8e7cc30 to 7836a53 Compare June 4, 2021 17:07
out = await self.run_command(cmd, input=script, env=self.get_env())
try:
self.log.info("Job submitted. cmd: " + cmd + " output: " + out)
self.log.info("Job submitted. output: %s", out)
Copy link
Member

Choose a reason for hiding this comment

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

If multiple spawners work at the same time, I figure this could be confusing to have written out in multiple separate lines - especially if there is a await in between that could make jobs print multiple messages that intermix.

Due to that, it may make sense to help the admin distinguish what spawner is printing what as well - then it would be without problem to remove logging cmd again in this place I think.

@mbmilligan
Copy link
Member

Hi, after we update the test matrix can you make a commit to this to trigger rerunning the CI?

@consideRatio
Copy link
Member

This looks reasonable, thank you @cmd-ntrf!

@consideRatio consideRatio merged commit fdd25e9 into jupyterhub:main Mar 16, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants