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

Logged notebook type #5425

Merged
merged 1 commit into from
May 6, 2020
Merged

Conversation

pinarkavak
Copy link

No description provided.

@pinarkavak pinarkavak mentioned this pull request May 5, 2020
Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

Great idea @pinarkavak - thank you. I just had some minor comments.

@@ -177,6 +177,7 @@ async def start_kernel(self, kernel_id=None, path=None, **kwargs):
self._kernel_connections[kernel_id] = 0
self.start_watching_activity(kernel_id)
self.log.info("Kernel started: %s" % kernel_id)
self.log.info("Kernel type: %s" % self._kernels[kernel_id].kernel_name)
Copy link
Member

Choose a reason for hiding this comment

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

Rather than introduce another INFO entry that might result in ambiguities when starting kernels simultaneously, would you be okay with extending Kernel started: <id> with a comma-separated "type" clause: Kernel started: <id>, type: <type>?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm - wondering if name: might be more indicative and closer to what we use in the code?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks Kevin, great points, my first intention was not to change any existing log, but updated the code to address your comments.

@@ -176,7 +176,7 @@ async def start_kernel(self, kernel_id=None, path=None, **kwargs):
kernel_id = await maybe_future(self.pinned_superclass.start_kernel(self, **kwargs))
self._kernel_connections[kernel_id] = 0
self.start_watching_activity(kernel_id)
self.log.info("Kernel started: %s" % kernel_id)
self.log.info("Kernel started %s, name: %s" % (kernel_id, self._kernels[kernel_id].kernel_name))
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the updates Pinar. I hate to nit, but I think the : following started should probably be retained. The only reason is in case there are "log scrappers", they'll probably be looking for Kernel started: - which is along the lines of your thoughts about preserving log entries. I suspect any applications that perform log analysis and need the kernel start time would be targeting "Kernel started:" as their token and not so much the content to EOL.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks Kevin! I totally agree, I somehow removed it without noticing while updating the log, added it back.

@pinarkavak pinarkavak force-pushed the log-notebook-type branch from 9b93626 to 8c063c2 Compare May 6, 2020 14:57
Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

LGTM - thanks Pinar!

@kevin-bates kevin-bates merged commit a291f31 into jupyter:master May 6, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants