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

Refactor the JWA backend to utilize common code #5316

Merged

Conversation

kimwnasptd
Copy link
Member

first step for #5310

Will move the backend under /components/crud-web-apps/jupyter/backend.

All the backend requests will have the following structure:

{
  "some-field": "some-value",

  "status": 200,
  "success": true,
  "user": "kimwnasptd",
  "log": "this field will be present only when the success is false",
}

Need to append a commit for tolerations and affinity.

We (Arrikto) are deploying this jupyter web app on MiniKF and have extensively tested it. So @elikatsis @yanniszark and @StefanoFioravanzo will most probably not have something big to propose in the review.

cc @thesuperzapper

Update the common python wheel wrt:
* How to distinguish between dev and prod mode
* Extra routes for handling Notebooks
* Serving the index.html for every non api route (SPA)
* Add a STOPPED state to the possible Status values

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
@kubeflow-bot
Copy link
Contributor

This change is Reviewable

* Extend the configuration yaml with default form values for the
  affinity/tolerations
* Set them accordingly when the user submits a notebook

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
Reviewed-by: Ilias Katsakioris <elikatsis@arrikto.com>
@kimwnasptd kimwnasptd changed the title WIP: Refactor the JWA backend to utilize common code Refactor the JWA backend to utilize common code Sep 23, 2020
@kimwnasptd
Copy link
Member Author

The last commit for supporting Affinity and Tolerations is there. This is ready for review



# helper functions
def start_stop_notebook(namespace, notebook, request_body):
Copy link
Member

Choose a reason for hiding this comment

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

Why does this need to be a separate function?

The patch function is sufficient?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking that If in the future we want to handle other cases in the PATCH request, then each one of them should be its own function and not have all the logic unfolded in the PATCH handler

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking that If in the future we want to handle other cases in the PATCH request, then each one of them should be its own function and not have all the logic unfolded in the PATCH handler

@thesuperzapper
Copy link
Member

Should we rename the /jupyter prefix on the URL to something more generic?

@thesuperzapper
Copy link
Member

This looks pretty good as a first pass (although I am still sceptical we need a fully separate code path for ROK).

I assume we will implement the changes from #5280 when we make the new notebook-front-end, as there are lots of backend changes required for those frontend changes.

Copy link
Member Author

@kimwnasptd kimwnasptd left a comment

Choose a reason for hiding this comment

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

@thesuperzapper

Should we rename the /jupyter prefix on the URL to something more generic?

Was thinking of keeping it as is for now until we replace the the code entirely. Then once we have completely replaced the app we can change the naming both in the routes as well in the manifests naming.

@kimwnasptd
Copy link
Member Author

I think we can move on with merging this.
Next step will be to submit the PR for the frontend

@elikatsis
Copy link
Member

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elikatsis

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

@k8s-ci-robot k8s-ci-robot merged commit 6eb007d into kubeflow:master Sep 29, 2020
2 of 3 checks passed
@kimwnasptd kimwnasptd deleted the feature-kimwnasptd-jwa-backend-refactor branch December 23, 2020 11:41
saffaalvi pushed a commit to StatCan/kubeflow that referenced this pull request Feb 11, 2021
* WA: Backend common: update the library

Update the common python wheel wrt:
* How to distinguish between dev and prod mode
* Extra routes for handling Notebooks
* Serving the index.html for every non api route (SPA)
* Add a STOPPED state to the possible Status values

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

* JWA: Add the refactored backend

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

* JWA: Backend: Add support for Affinity/Tolerations

* Extend the configuration yaml with default form values for the
  affinity/tolerations
* Set them accordingly when the user submits a notebook

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
Reviewed-by: Ilias Katsakioris <elikatsis@arrikto.com>
saffaalvi pushed a commit to StatCan/kubeflow that referenced this pull request Feb 12, 2021
* WA: Backend common: update the library

Update the common python wheel wrt:
* How to distinguish between dev and prod mode
* Extra routes for handling Notebooks
* Serving the index.html for every non api route (SPA)
* Add a STOPPED state to the possible Status values

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

* JWA: Add the refactored backend

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

* JWA: Backend: Add support for Affinity/Tolerations

* Extend the configuration yaml with default form values for the
  affinity/tolerations
* Set them accordingly when the user submits a notebook

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
Reviewed-by: Ilias Katsakioris <elikatsis@arrikto.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants