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

Rewrite the Central Dashboard in Angular #38

Open
thesuperzapper opened this issue Jun 21, 2021 · 23 comments
Open

Rewrite the Central Dashboard in Angular #38

thesuperzapper opened this issue Jun 21, 2021 · 23 comments

Comments

@thesuperzapper
Copy link
Member

/kind feature

Currently, most of our web apps use Angular (CRUD Jupyter/Volumes/Tensorboards), however the Central Dashboard is written in Polymer.

In the interest of consistency and maintainability, we should rewrite the Central Dashboard in Angular.

cc @kubeflow/wg-notebooks-leads

@thesuperzapper
Copy link
Member Author

cc @davidspek

@thesuperzapper
Copy link
Member Author

/priority p3

@davidspek
Copy link

Agree, this is extremely important in terms of maintainability and making it accessible for people to contribute to the central dashboard.

KFServing and Katib web apps are also Angular.

@kimwnasptd
Copy link
Member

In the interest of consistency and maintainability, we should rewrite the Central Dashboard in Angular.

@thesuperzapper could you elaborate more on this? The Central Dashboard is fundamentally different from the CRUD web apps, so there's minimal code that could be leveraged from the common code of the CRUD web apps.

I'm very skeptical about even considering to put the effort on such a huge effort, considering the very minimal benefits it would provide.

@thesuperzapper
Copy link
Member Author

@kimwnasptd I am aware that CRUD is not similar, but I believe there will be common stuff which can be shared if all the web apps are written in the same framework.

But fundamentally, the main benefits are:

  • standardisation - we don't have to use different packages (for things like translation)
  • maintainability - we only need to be experts in a single framework, rather than 2
  • features - angular is a lot more popular than polymer, so has more options/plugins
  • security - we reduce the possible attack surface by using a single framework

I am aware it's going to be a bit of work to do a rewrite, but this work will only get bigger as we start adding features to Central Dashboard.

@stale
Copy link

stale bot commented Mar 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot closed this as completed Apr 16, 2022
@thesuperzapper
Copy link
Member Author

thesuperzapper commented Aug 29, 2022

Reopening this as a central location for us to track work on rewriting the central dashboard in Angular.

There was a good discussion in kubeflow/kubeflow#6332 until it was closed, the main takeaway was that we need a design doc (possibly in the form of a proposal) that outlines the following things:

/reopen
/cc @kimwnasptd @haoxins @sanjshres

@google-oss-prow
Copy link

@thesuperzapper: Reopened this issue.

In response to this:

Reopening this a central location for us to track work on rewriting the central dashboard in Angular.

There was a good discussion in kubeflow/kubeflow#6332 until it was closed, the main takeaway was that we need a design doc (possibly in the form of a proposal) that outlines the following things:

/reopen
/cc @kimwnasptd @haoxins @sanjshres

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@google-oss-prow google-oss-prow bot reopened this Aug 29, 2022
@thesuperzapper
Copy link
Member Author

/lifecycle frozen

@thesuperzapper
Copy link
Member Author

We had a discussion with @kimwnasptd @sanjshres in today's Notebook WG meeting, and this is a summary.

Next Steps

  1. Define the list of functional requirements that the existing central-dashboard has
  2. Decide on a base UI template (Bootstrap, etc)
  3. Create a prototype/scaffold and merge it into a new folder under components of kubeflow/kubeflow repo

Functional Requirements (by page)

  • Overview Page
    • Recently opened notebooks
    • etc
  • Main Page
    • (always present)
      • Logout button
      • Namespace Selector
      • Sidebar Menu
      • A config map controls to control the list of items on the sidebar
    • (with sidebar item selected)
      • iFrame Window
      • selected namespace is passed via URL arg
  • Manage Contributors Page
    • Add/Remove Contributors

@kimwnasptd
Copy link
Member

An update on this. Before talking about how to break down the frontend changes we'll need to discuss how to actually have some Angular code in the first place.

Right now the Polymer frontend and NodeJS backend code are tightly coupled inside the components/centraldashboard folder. So we'll need to first decouple these 2 to then be able to have a dedicated folder for the frontend.

As a first step I propose that we create a new components/centraldashboard-angular app, to start porting the Angular code there without breaking the existing codebase. In my mind the end result will be very similar to what we have in the crud-web-apps:

centraldashboard-angular/
    backend/    # The current NodeJS backend code
    frontend/   # The new Angular frontend code
    manifests/  # The current manifests
    Dockerfile
    Makefile
    README.md

Lastly, a reminder that the scope of this effort is to keep the current backend code as is (for now?) and only focus on rewriting the frontend part in Angular.

@kimwnasptd
Copy link
Member

After some discussion we had with @orfeas-k we will do the following next steps, for establishing an Angular codebase that we can start working collaboratively on:

  1. Ensure the current backend instructions can work locally
  2. Decouple the existing frontend and backend code. We'll need to break down some "shared" files like package.json
  3. Create a new empty Angular project under centraldashboard-angular/frontend and configure a proxy.json for using the Angular dev server alongside the backend and proxied services (i.e. Jupyter web app)
  4. Create the necessary Dockerfile and Makefile for building the new app

@jbottum
Copy link

jbottum commented Nov 15, 2022

/priority P2

@kimwnasptd should these updates be tagged as P1 for v1.7 ?

@kimwnasptd
Copy link
Member

@kimwnasptd should these updates be tagged as P1 for v1.7 ?

@jbottum I don't think this effort will have progressed to the point we could recommend users to try it out for 1.7. So I wouldn't tag this as P1, but definitely P2

@orfeas-k
Copy link

orfeas-k commented Dec 7, 2022

IFrame URL tracking (change detection)

Context

One of the key functionalities of the CentralDashboard is the communication between itself and the iframe in which the WAs are rendered. Part of this communication is the URL "syncing" during which, the CentralDashboard needs to track the iframe's URL at all times and mirror changes in the UI and the browser's URL.

This communication can be broken down into the following parts:

  1. The dashboard passes the relative URL* it wants the iframe to use.
  2. The dashboard detects changes of the iframe's internal location. Some use case scenarios are:
    • User clicks on a link and navigates to a new page inside the WA they are e.g. navigates to the notebooks details page in JWA
    • User clicks on a link that navigates to another WA from the one they were previously in e.g. navigates to a JWA notebook from inside VWA details page
    • User clicks affect the query parameters of the current URL e.g. navigates to another tab inside a WA's details page
  3. The dashboard syncs its URL (the one the user sees in the browser) to mirror the iframe's internal URL.

* We consider that values for the sidenav menu in centraldashboard's configmap contain only relative URLs.

Implementation details

Part 1

This is easily done by using iframe's src property. In order to update this when the Central dashboard window.location.href changes, we subscribe to router.events that emits events every time the router changes and this then updates the src. However, in order to avoid updating the src and reloading the iframe on every user click, we will check if the new router.url that is sent through router.events is the same as the iframe's internal window URL. In the case these two are the same, we will skip it.

Part 2

After trying many different approaches, we ended up simplifying the implementation by checking the iframe's internal window.location.href at time intervals (every 100ms). We will keep its previous value saved and only act upon value change.

Options that were also considered:

  • Bind the src property of the <iframe>. This didn't work. As it seems, the src property doesn't change when the user navigates inside the iframe.
  • Add an eventListener to the iframe's window for popState. This however, didn't help since it gets triggered only when the user clicks a back or forward button https://developer.mozilla.org/en-US/docs/Web/API/Window/popstate_event#the_history_stack
  • Use the onLoad event ( load in Angular) but this is triggered only when the <iframe> is first loaded.
  • Use a MutationObserver to detect changes of the iframe's internal window.location.href in the DOM tree. This worked but with a second thought, it didn't make much sense to check the DOM in order to detect the changes in the iframe's location object (which is not a DOM element). At the same time, we ended up checking really often since the MutationObserver callback function was being called many times per second.
  • Imitate how Angular checks for location changes. It looks like though that they make an explicit decision of using popstate when they change URLs in Angular. But this doesn't seem to be a way for us to detect changes in location uniformly

Part 3

We use router.navigate() in order to update the Central Dashboard's URL. This triggers router events and calls the logic to update the src property as well.

See this PR for more technical implementation details.

Update
Here is a graph showing the above design
image

@wg102
Copy link

wg102 commented Mar 4, 2024

Two questions :

  1. Has there being a deadline for the angular rewrite of central-dashboard?
  2. Is there a branch to contribute to if wanting to contribute to the effort? Or we make one, do a bit of the work and merge it direct to master?

@thesuperzapper
Copy link
Member Author

@wg102 we are definitely open to more contributions, if you are interested in helping, we should probably discuss a plan so people don't waste their efforts.

It looks like @orfeas-k is no longer actively making PRs, so It you want to look at the current state of ./components/centraldashboard-angular in master, and propose some next steps, please do!

Please comment here, after investigating the current state (to help us understand how much work is left), or join the next Notebooks WG meeting to show us.

@StefanoFioravanzo
Copy link
Member

It's wonderful to see some traction on this! @wg102 welcome! It seems like the rewrite project was pretty far along. Hopefully @orfeas-k can provide a summary of where things were left off.

Just for context, I believe the initial goal was to implement a new fronted that has feature parity with the old (current) one. We had a few aspirational goals such as making sure that the sidebar menu is configurable and has drop down/nested items support, or that it's collapsible with a nice icon only / icon+text mode. Not sure how much of that was already done

@orfeas-k
Copy link

orfeas-k commented Mar 5, 2024

@wg102 @thesuperzapper @StefanoFioravanzo Good to see some traction on this as well. I 'll try to allocate some time in the coming days to get up to speed again and summarize the state so this can handed over to someone, since I did not have the time to work on this during the past many months.

@orfeas-k
Copy link

Current state

Things that have been done until now:

Note that there are PRs (linked to this issue) that were not mentioned since they are minor fixes/steps along the way.

Multi user

As the PRs mention in KF version PR and Add namespace selector PR, at the time of coding this, my understanding was that no distribution uses multi-user and single-user values anymore from the backend API endpoint /env-info . This led me to implement those features accordingly. However, discussions later lead me to think that this assumption was wrong, and thus those two parts of the CDB may need to be updated to account for those cases too, in order to ensure we do not break anything.

What's left

From the above description, the things that seem to be left are:

  • All the logic around multi-user usage of kubeflow (the convention I 'm describing above) in the features already implemented.
  • Manage contributors page
  • Registration page
  • Home(overview) page. This is the one that users are taken after registering.
  • Logout button
  • Stylizing the dashboard. This shouldn't be too much of an effort since all the WAs follow some common styles and are based on Material. There has already been an effort AFAIR about common styles in #7062

That's my best estimation and reality shouldn't be too far from that. That being said, I think that although the fundamentals have been set. there is still quite some work to be made.

Contributing

These are my 2 cents. I 'm really glad to see that this is moving forward and I 'm happy to provide any more guidance to someone that works on it.

Regarding:

Or we make one, do a bit of the work and merge it direct to master?

Since we 're contributing to a new project that is not being used by Kubeflow right now, contributing directly to master would not cause any harm.

@andreyvelich
Copy link
Member

/transfer dashboard

Copy link

@thesuperzapper: The label(s) kind/feature cannot be applied, because the repository doesn't have them.

In response to this:

/kind feature

Currently, most of our web apps use Angular (CRUD Jupyter/Volumes/Tensorboards), however the Central Dashboard is written in Polymer.

In the interest of consistency and maintainability, we should rewrite the Central Dashboard in Angular.

cc @kubeflow/wg-notebooks-leads

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@google-oss-prow google-oss-prow bot transferred this issue from kubeflow/kubeflow Nov 11, 2024
@tariq-hasan
Copy link

@thesuperzapper Regarding the remainder of the tasks in the last comment I have the following questions:

  • What is the prioritization around these tasks?
  • Do we have Github issues to track each of these individual tasks?

I was also wondering what the plan would be to test out the completed Angular rewrite in parallel with the Polymer version of the dashboard and how the Angular rewrite would eventually be planned to be rolled out.

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

9 participants