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

⚡ Include the jupyter-server-proxy for tensorboard function #11

Merged
merged 1 commit into from
Jul 26, 2022

Conversation

harshad16
Copy link
Member

@harshad16 harshad16 commented Jul 15, 2022

  • Include the jupyter-server-proxy for tensorboard function
  • Update the OWNERS of RHODS for repo
  • Update the CI tests

Co-authored-by: Vaishnavi Hire vhire@redhat.com
Signed-off-by: Harshad Reddy Nalla hnalla@redhat.com

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • JIRA link(s): https://issues.redhat.com/browse/RHODS-2978
  • The Jira story is acked
  • An entry has been added to the latest build document in Build Announcements Folder.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious)

 - Update the OWNERS of RHODS for repo
 - Update the CI tests

Co-authored-by: Vaishnavi Hire <vhire@redhat.com>
Signed-off-by: Harshad Reddy Nalla <hnalla@redhat.com>
@sesheta
Copy link

sesheta commented Jul 15, 2022

Pre-Commit Test failed! Click here
[INFO] Initializing environment for https://github.com/Lucas-C/pre-commit-hooks.
[INFO] Initializing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Installing environment for https://github.com/Lucas-C/pre-commit-hooks.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
An unexpected error has occurred: CalledProcessError: command: ('/opt/app-root/src/.cache/pre-commit/repodcgk0e_5/py_env-python3.6/bin/python', '-mpip', 'install', '.')
return code: 1
expected return code: 0
stdout:
    Processing /opt/app-root/src/.cache/pre-commit/repodcgk0e_5
    
stderr:
        ERROR: Command errored out with exit status 1:
         command: /opt/app-root/src/.cache/pre-commit/repodcgk0e_5/py_env-python3.6/bin/python -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/opt/app-root/src/.cache/pre-commit/repodcgk0e_5/setup.py'"'"'; __file__='"'"'/opt/app-root/src/.cache/pre-commit/repodcgk0e_5/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' egg_info --egg-base /tmp/pip-pip-egg-info-buvojebx
             cwd: /opt/app-root/src/.cache/pre-commit/repodcgk0e_5/
        Complete output (6 lines):
        Traceback (most recent call last):
          File "<string>", line 1, in <module>
          File "/opt/app-root/src/.cache/pre-commit/repodcgk0e_5/setup.py", line 1
            from __future__ import annotations
            ^
        SyntaxError: future feature annotations is not defined
        ----------------------------------------
    ERROR: Command errored out with exit status 1: python setup.py egg_info Check the logs for full command output.
    
Check the log at /opt/app-root/src/.cache/pre-commit/pre-commit.log

@VaishnaviHire
Copy link

Tested custom build for JupyterHub and Notebook Controller:

For JupyterHub:

Users need to set following env variable in Jupyterlab:

import os
os.environ["TENSORBOARD_PROXY_URL"]="/user/<user-id>/proxy/6006/"

Result:
Screen Shot 2022-07-15 at 3 31 43 PM


For Notebook- Controller

Users need to set following env variable in Jupyterlab:

import os
os.environ["TENSORBOARD_PROXY_URL"]="/notebook/<nb-namespace>/<nb-name>/proxy/6006/"

Result:
Screen Shot 2022-07-15 at 4 42 16 PM

@lugi0
Copy link

lugi0 commented Jul 18, 2022

@VaishnaviHire Is there a way to create these env vars automatically when the pod is being spawned?
The manual step of adding the proxy can be given if e.g. the user wants to use a non-standard port for tensorboard, but otherwise I think this should be already set

@samuelvl
Copy link

@lugi0 For now this will be documented as a manual step of the TensorFlow spawning process but definitely yes, we want to inject this variable in the UI spawner.

@VaishnaviHire Can you open a issue in the dashboard repository to track this feature?

@VaishnaviHire
Copy link

VaishnaviHire commented Jul 18, 2022

Dashboard Issue: opendatahub-io/odh-dashboard#314

cc: @lugi0 @samuelvl

@lugi0
Copy link

lugi0 commented Jul 18, 2022

@VaishnaviHire Do you know if this fix will work for the PyTorch image as well? the issue is present there too, and we explicitly mention Tensorboard in the list of provided libraries for that image

@VaishnaviHire
Copy link

@VaishnaviHire Do you know if this fix will work for the PyTorch image as well? the issue is present there too, and we explicitly mention Tensorboard in the list of provided libraries for that image

This Dashboard PR, will set the env variable for both Tensorflow and Pytorch notebooks, however we would need to add jupyter-server-proxy to pytorch image as well. I will open the PR.

Comment on lines -5 to +4
base-image: "quay.io/thoth-station/s2i-minimal-notebook:v0.0.6"
base-image: "quay.io/thoth-station/s2i-minimal-py38-notebook:v0.3.0"
Copy link

Choose a reason for hiding this comment

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

wasn't pytorch's base image the minimal CUDA one? or is this something else?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just for normal build testing, this doesnt go into the final image.
It is just used for the build-check in the github PR, as you can see below.
and yes, CUDA minimal image used based in the build pipeline finally.

@lugi0
Copy link

lugi0 commented Jul 22, 2022

Tested and working well, hold on merging until red-hat-data-services/ods-ci#470 is merged

Copy link

@lugi0 lugi0 left a comment

Choose a reason for hiding this comment

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

Automation is merged, this can move ahead

@samuelvl samuelvl merged commit 45e5e50 into red-hat-data-services:python38 Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants