Skip to content

Debug local upgraded KF1.7 jupyter-apis #237

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

Closed
4 tasks done
mathis-marcotte opened this issue May 30, 2023 · 8 comments
Closed
4 tasks done

Debug local upgraded KF1.7 jupyter-apis #237

mathis-marcotte opened this issue May 30, 2023 · 8 comments
Assignees

Comments

@mathis-marcotte
Copy link
Contributor

mathis-marcotte commented May 30, 2023

EPIC StatCan/aaw#1632

Make jupyter-apis 1.7 able to run, and then test to report all the issues(or lack of issues) with the new 1.7 upgrade.

Things to note

  • Make sure dialogs work:
    Looks like they worked while testing

  • jupyter/frontend/src/app/pages/form/form-new/form-cpu-ram/form-cpu-ram.component.html
    In this file, we don't use the lib-positive-input component like upstream, but it is not mentionned why. So maybe it might be a good idea to test again to see if whatever issue was present is now fixed, or at least find out what the problem is so that we can comment that in the code:
    Looking at it, it just seemed a lot more restrictive to use the lib-positive-input compared to just the normal input component.

  • jupyter/frontend/src/app/pages/form/form-new/volume/existing/pvc/pvc.component.ts
    It seems like the term "PVC" and "Volume" gets used interchangibly. So in this file it seems like there was a difference between a function that used to be "getPVCS" is now "getVolumes" so we just need to make sure everthing still works.:
    Set it all to "getPVCs" to match more with upstream

  • jupyter/frontend/src/app/pages/form/form-new/volume/mount/mount.component.ts
    Quite a lot seemed to have changed in this file, so we just need to make sure that all the validators are still applied correctly(and this is also applacable to all the other inputs that we need to make sure validation is working):
    Looks like all the validators worked while testing

@mathis-marcotte mathis-marcotte transferred this issue from StatCan/aaw May 30, 2023
@mathis-marcotte mathis-marcotte self-assigned this May 31, 2023
@mathis-marcotte mathis-marcotte changed the title Test the upgraded KF1.7 jupyiter-apis Debug local upgraded KF1.7 jupyiter-apis May 31, 2023
@mathis-marcotte mathis-marcotte changed the title Debug local upgraded KF1.7 jupyiter-apis Debug local upgraded KF1.7 jupyter-apis May 31, 2023
@mathis-marcotte
Copy link
Contributor Author

Upstream introduced a new function to get the value of the current namespace. So now we have getSelectedNamespace and getSelectedNamespace2, which the only difference being that the former returns a String while the latter returns a String | String[]. Now, other than the fact that "getSelectedNamespace2" is REALLY not a readable name for the function, we dont allow the "all namespaces" value to be selected as we only allow 1 namespace to be selected a time, and therefore don't need to use a string array for namespace value. So to avoid some issues with our custom functions that only expect a string value and not a string array, we will just use getSelectedNamespace instead of getSelectedNamespace2.

@mathis-marcotte
Copy link
Contributor Author

Got jupyter-apis running, as in following all the start-up instructions and the npm start is not failing.

Most initial issues were just related to making sure our custom code worked with the updated upstream, as well as making sure what got imported from the "Volumes" upstream app worked with our jupyter app.

@mathis-marcotte
Copy link
Contributor Author

mathis-marcotte commented Jun 2, 2023

Index Page

On first glance, things look not too too bad, we the data is being displayed.
image

Some issues i can first notice are:

  • the "new volume" button could go right above the volume table instead of being next to the "new notebook" button. This is something that can be decided.:
    We are hiding this button to hide the volume form
  • the blob-csi volumes like "aaw-unclassified" are displayed where as we used to hide those ones :
    code that got removed
  • there is a different behavior with the scroll bars where each table has their own scrollbar instead of just having the global page one. (This is an issue that I think we fixed in the past so it might just be related to styling that we need to re-apply) :
    styling was added that made it look weird since WE have multiple tables in the index.
  • the common table component got updated in 1.7 to have filtering. This functionality seems to be done by default. As for our own Kubecost table, this filtering feature seems excessive/unecessary for that table. So we might want to either use another simpler table component if available, or make our own simple table component to display the kubecost data.:
    We wont change this in this ticket, but its something that will be noted down in the 1.7 epic as something to revisit in the future.
  • "Created at : NaN years ago" for all the notebooks. :
    This seemed to be caused because we were formatting the datetime in the backend, but the frontend is now expecting datetime value to format it.
  • Notebooks "Last activity" is always empty :
    this is not related to the 1.7 update. A new issue was created to handle this.
  • The links in the "Used by" column in the volumes table don't work. They just redirect you back to localhost(testing locally) with no page getting rendered. :
    The issue was that the link url was prefixed by "/jupyter", which didnt fit with our routing.

In the console
image
(seems like this also happened before the 1.7 update, and is only an error while running the app locally)

image

Yeah, the time is really messed up
(but should now be fixed)

@mathis-marcotte
Copy link
Contributor Author

mathis-marcotte commented Jun 2, 2023

New Notebook Form

Looking at the new notebook form. This is me just quickly looking through without me playing with anything.
image

  • since we have 1 more image type offered compared to upstream, we see that it was not styled for 4 images and it cuts off. One thing we could potentially do to give ourselves a bit more room to work with is to not limit the form size to only 50% of the screen which is what it looks like is happening. :
    For this issue, we can possibly look at 2 options : to extend just the image selection part of the form to have it take the space it needs, or to have it set up in a 2x2 grid instead of a 4x1
    Decided to extend the image selection. We were doing this previously to 1.7, the styling was just lost in the update.

  • The images boxes are not accurate. R studio icon with the visual studio code title. Ubuntu icon with the rstudio text.

image

  • When a custom notebook is checked, the image type selection breaks. So that would need to be fixed. :
    The way the form images are displayed changed and i think this bug was caused by that change combined with our custom code.

  • The only image type that has images available under the "Custom Notebook" > Image dropdown is the Jupyter image.:
    The values given to the component to fill that input were wrong. It expected a config object array but it was being given a string array

image

  • default "16Gi" is not being applied to the default new workspace volume. (Although i guess it is somewhat applied if we see it in the top bar for the volume next to the volume name):
    Our custom code to set the default to 16 had gotten removed, so just needed to add that back

image

  • Default language is not set in the dropdown. Might be a similar issue as the default volume sizes not being set in the select inputs:
    Angular source default was set to en-US, but in our dropdown we only support en.

@mathis-marcotte
Copy link
Contributor Author

mathis-marcotte commented Jun 2, 2023

New Volume Form

New volume form looks like it's going to do what it's going to do. Although we may not want to actually keep this feature, so take the reported issues lightly as we may just scrap the whole thing.
image

  • We will probably want to alter the default size to 16 like when creating a new volume through the "New Notebook" form, as well as change the input from a number box to a dropdown of values.
  • We may want to add a mount path input as well.

Decision

We will hide this feature for now as it doesn't seem like a core feature that needs to be included right away. Let's hide it for now and re-evaluate later.

  • Hide this feature:
    Hid the button to make this form unaccessible.

@mathis-marcotte
Copy link
Contributor Author

mathis-marcotte commented Jun 2, 2023

This error randomly appeared in the console
image

I noticed it after closing the "New Volume" form, but I don't think that is the exact cause.

Something to investigate!

Investigation

This was caused by the "Created at" values. It was failing to format the datetime because we were already formatting it in the backend. So decided to just return datetime from the backend.

@mathis-marcotte
Copy link
Contributor Author

mathis-marcotte commented Jun 2, 2023

Notebook details

image

Some things to note

  • looks like the /pod api call is failing:
    You know, getting a 404 on the api call for the pod info when you DON'T have a pod running makes sense. So this isn't an issue, but the intended behavior(it seems).
  • I've seen the "Configurations" value get duplicated. So in the above picture, there was multiple "protected-b" links shown. I am not sure how to reproduce this, but just letting the page open seemed to have that eventually happen. :
    Upstream doesn't seem to have this issue from my testing. But moved the logic of the value getting popuated to try and fix/mitigate this issue.
  • The PVC link isnt working:
    The url was prefixed with /volumes and that didn't work with our routing

image

  • Conditions table looks like it might not be loading properly :
    This is because the struct defined in the package we are using was missing some fields neede to populate this table. A PR was made to update the package to fix this issue. Updated NotebookCondition struct kubeflow-apis#1
    PR was merged and the issue is fixed

image

Logs seem to work

image

Events seem to work

image

  • Yaml doesn't seem to be working :
    This seems to be caused by the monaco-editor not being setup properly. And the cause of that seems to be tied to the differences with our backend setup compared to upstream. :
    The issue seemed to be cause because we are using a different proxy-config file than upstream, and our proxy-config file was missing a config.

@mathis-marcotte
Copy link
Contributor Author

mathis-marcotte commented Jun 2, 2023

Volume Details

image

  • When a Pod is mounted, a link to the notebook is there, but the link isn't working:
    The links were prefixed with something that didnt work with our routing

image

Events seem good.

image

  • Yaml doesn't seem to work :
    Same issue as with the notebook details page

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

No branches or pull requests

1 participant