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

Add execution start and end time metadata for code cells #29

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Darshan808
Copy link

References #22

  • Added execution start and end times to code cell metadata, visible when timing recording is enabled by user.
  • Computed on the server side to ensure persistence even after the notebook is closed.

Screenshots

image

Copy link

github-actions bot commented Jan 7, 2025

Binder 👈 Launch a Binder on branch Darshan808/jupyter-server-nbmodel/add-execution-timing-metadata

@krassowski krassowski added the enhancement New feature or request label Jan 7, 2025
Copy link
Collaborator

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

🚀 I think we would want to add a test, for example see:

async def test_post_execute(jp_fetch, pending_kernel_is_ready, snippet, output):

since the exact timestamp will vary I would just check that the timing metadata is present and that the end time is greater than start time.


if metadata["record_timing"]:
time_info = ycell["metadata"].get("execution",{})
time_info["start_time"] = datetime.now(timezone.utc).isoformat()[:-6]
Copy link
Collaborator

Choose a reason for hiding this comment

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

For the metadata I think we will want to match the exact identifiers as used on the frontend, so instead of start_time we would use shell.execute_reply.started and for end time - shell.execute_reply - although these are less readable, it would make jupyter-server-nbmodel a drop-in replacement and compatible with existing extensions like jupyterlab-execute-time.

There also should be execution_failed for when the execution fails.

@Darshan808
Copy link
Author

I think we would want to add a test, for example see:

For testing whether the metadata is being added correctly, we would need to pass ydoc: The YDoc jupyter server extension and metadata: Execution context. But during testing, we pass None and {} as their values due to which we get response from server but cell metadata is not added.

Can we somehow create dummy ydoc and metadata and pass it instead or do we have some other better way to do this?

@krassowski
Copy link
Collaborator

krassowski commented Jan 8, 2025

Can we somehow create dummy ydoc and metadata and pass it instead or do we have some other better way to do this?

You can change config to something like:

"ServerApp": {
    "jpserver_extensions": {
        "jupyter_server_ydoc": True,
        "jupyter_server_nbmodel": True,
        "jupyter_server_fileid": True,  # from jupyter_server_ydoc
    },
    "token": "",  # not sure if needed
    "disable_check_xsrf": True,   # not sure if needed
},

in

pytest_plugins = ("pytest_jupyter.jupyter_server",)
@pytest.fixture
def jp_server_config(jp_server_config):
return {
"ServerApp": {
"jpserver_extensions": {"jupyter_server_nbmodel": True, "jupyter_server_ydoc": False}
}
}

to enable jupyter_server_ydocand then I think you can access it from jp_serverapp.web_app.settings["jupyter_server_ydoc"] as in here, or using self.serverapp.extension_manager.extension_apps as in

RTC_EXTENSIONAPP_NAME = "jupyter_server_ydoc"
STOP_TIMEOUT = 3
_request_id_regex = r"(?P<request_id>\w+-\w+-\w+-\w+-\w+)"
class Extension(ExtensionApp):
name = "jupyter_server_nbmodel"
def initialize_handlers(self):
rtc_extension = None
rtc_extensions = self.serverapp.extension_manager.extension_apps.get(
RTC_EXTENSIONAPP_NAME, set()
)

You may also need to create a real collaborative notebook, for that purpose add jupyter_server_ydoc.pytest_plugin which will pull fixtures from https://github.com/jupyterlab/jupyter-collaboration/blob/main/projects/jupyter-server-ydoc/jupyter_server_ydoc/pytest_plugin.py and allow you to use rtc_create_notebook.

metadata: Execution context

Is this the same as we are sending from frontend? If so you should be able to populate it with cell_id, document_id and "record_timing": True.

@Darshan808 Darshan808 requested a review from krassowski January 9, 2025 10:49
conftest.py Outdated Show resolved Hide resolved
conftest.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@Darshan808
Copy link
Author

This CI error seems similar to the one described in the jupyterlab/jupyter-collaboration#252.

@krassowski
Copy link
Collaborator

I think these are warnings, so I would not be too worried. I would focus on fixing

ERROR jupyter_server_nbmodel/tests/test_handlers.py::test_execution_timing_metadata - KeyError: 'file_id_manager'

@Darshan808
Copy link
Author

I've spent considerable time and tried various methods to resolve the issue, but I still encounter an error stating "KeyError: 'event_logger'" in traitlets.
Screenshot from 2025-01-10 08-13-26

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants