-
Notifications
You must be signed in to change notification settings - Fork 8
Rsdev 782 jupyter notebooks #49
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
Conversation
… Hub. Password secret works in Hub and save of notebook works for both
…to store variables
…solution for JupyterLite
…doc if they match saved ids for jupyter_notebook or its data_files
…d in notebook cell
…file by sending twice to rspace
…okname, server url and server port
…of notebook has not changed
…into a given field
…g notebook to gallery twice
…ted in sync_notebook_to_rspace function
| This must be set to the name of the rspace user where the notebook is being saved | ||
| """ | ||
|
|
||
| def get_server_urls(): |
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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('.')], |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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?
… same notebook as being synced and is python
No description provided.