Skip to content
This repository has been archived by the owner on Aug 17, 2023. It is now read-only.

[WIP] Make fairing work with JupyterHub #7

Closed
wants to merge 116 commits into from

Conversation

wbuchwalter
Copy link
Contributor

@wbuchwalter wbuchwalter commented Nov 6, 2018

This is an attempt to bypass the various limitations introduced by JupyterHub.
@jlewi @r2d4 Is there already a timeline for when Kubeflow will migrate from JupyterHub to the custom notebook controller?
If yes, Is this PR even needed?

Note: The history is a bit weird because kubeflow/fairing is not a fork of wbuchwalter/fairing, but we can just squash everything.

Fixes #6


This change is Reviewable

Copy link
Member

@r2d4 r2d4 left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

shutil.copytree(context_dir, dst)

def get_mount_point(self):
return os.path.join(os.environ['HOME'], '.fairing/build-contexts/')
#return os.path.join('/tmp/fairing/build-contexts/')
Copy link
Member

Choose a reason for hiding this comment

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

remove comment


success = self.check_build_succeeded(bld)
if success:
logger.error('Build finished successfully.')
logger.warn('Build finished successfully.')
Copy link
Member

Choose a reason for hiding this comment

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

why is this message warn if the build was successful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So that users get a confirmation either in the console or in the notebook that their job was launched without having to change the verbosity of the logger.

Copy link

Choose a reason for hiding this comment

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

can info be default log-level?

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: r2d4
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: wbuchwalter

If they are not already assigned, you can assign the PR to them by writing /assign @wbuchwalter in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@r2d4
Copy link
Member

r2d4 commented Nov 6, 2018

This should look a lot cleaner on github if you rebase on the current master branch right?

@wbuchwalter
Copy link
Contributor Author

wbuchwalter commented Nov 12, 2018

@r2d4 I actually can't rebase since we squashed the initial PR. So I would need to go over the changes of each commits.
If it's too annoying for you lmk and I'll squash these commit into a new PR.

However, I am still not sure if we need this PR or not.

@jlewi
Copy link
Contributor

jlewi commented Dec 17, 2018

@wbuchwalter @r2d4 Any update on this PR? Can we close it? Or do we still need it?

@wbuchwalter
Copy link
Contributor Author

@jlewi Closing it. the new append builder works fine from JH so it's the recommended way to do that.

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.

5 participants