Skip to content

Conversation

@nhanlon2
Copy link
Contributor

No description provided.

… Hub. Password secret works in Hub and save of notebook works for both
…doc if they match saved ids for jupyter_notebook or its data_files
@nhanlon2 nhanlon2 requested a review from mKowalski256 October 24, 2025 08:29
@tchoi-rs tchoi-rs self-requested a review October 24, 2025 13:34
This must be set to the name of the rspace user where the notebook is being saved
"""

def get_server_urls():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are there tests for the utility functions within here? And do they need to be within the context of the sync_notebook_to_rspace function?

if server_port is not None:
srv_url = srv['url']
part_url = srv_url[:srv_url.rfind(':') + 1]
all_urls.append(part_url + str(server_port) + '/lab/tree/' + str(path))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prefer template strings

all_urls.append(part_url + str(server_port) + '/lab/tree/' + str(path))
else:
all_urls.append(srv['url'] + 'lab/tree/' + str(path))
except Exception:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Too broad exception

notebook_name_alone = notebook_name[notebook_name.rfind('/') + 1:]
else:
notebook_name_alone = notebook_name
return {'name': notebook_name_alone, 'root_name': notebook_name_alone[:notebook_name_alone.rfind('.')],
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would utilise dataclasses instead of objects (I see that you're using dill which requires Python 3.8 so Dataclasses should always be available)

from rspace_client.eln import eln
import os
import hashlib
import dill
Copy link
Collaborator

Choose a reason for hiding this comment

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

3rd party dependencies should be specified in pyproject.toml

return "password set"


async def sync_notebook_to_rspace(rspace_url="", attached_data_files="", notebook_name=None, server_url=None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Provide type hinting

with open(state_filename, 'rb') as f:
try:
loaded_state = dill.load(f)
except Exception as e:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the sync really fail silently?

@nhanlon2 nhanlon2 merged commit 3514573 into master Oct 27, 2025
3 checks passed
@nhanlon2 nhanlon2 deleted the RSDEV-782-Jupyter-Notebooks branch October 27, 2025 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants