-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Adds recent Pipelines cards for Definitions and Runs. #3379
Conversation
This commit is pushed to the test cluster (https://kf-dev.endpoints.prodonjs-kubeflow-dev.cloud.goog/) |
Looks soooo good!!!! I love it. Can you please change the title of the pipeline card to "Recent Pipelines". Pipelines are not always created, they're sometimes uploaded, saved from the Hub, in the future they'll be deployed directly from Notebooks... it's safer to just call it "Recent Pipelines" |
@@ -21,6 +21,10 @@ div#grid | |||
.column | |||
paper-card(heading='Recent Notebooks') | |||
notebooks-card(namespace='[[namespace]]') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notebooks-card(namespace='[[namespace]]') | |
notebooks-card(namespace='[[namespace]]') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack'ed.
paper-card(heading='Recently Created Pipelines') | ||
pipelines-card(artifact-type='pipelines') | ||
paper-card(heading='Recent Pipeline Runs') | ||
pipelines-card(artifact-type='runs') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to call these elements cards? In that case, maybe they should extend paper-card itself?
class PipelinesCard extends PaperCard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You know I never thought of that. Going with the inheritance route doesn't seem like it would provide much benefit since the main purpose of the paper-card
is to get the elevation style and template which exposes the slot where our content goes. I experimented with it and it seemed like a bit more trouble than it's worth, especially since our overridden styles for paper-card wouldn't apply to the new element's name.
We could have the paper-card
be part of the template for the pipelines-card
and notebooks-card
items, but we'd need to move some styles around to accommodate that.
It would probably be more explicit to say pipelines-card-body
but I got lazy with naming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, yeah both are valid. Ok let's go with the body name in that case?
@avdaredevil I was able to move most of the common card stuff to the the card-styles module and then move the Changes are deployed to the cluster so LMK what you think. |
@@ -206,6 +206,10 @@ module.exports = { | |||
'/$1/services/$2/proxy/notebook/$1/$2/$3', | |||
}, | |||
}, | |||
'/pipeline': { | |||
target: 'http://localhost:8083/api/v1/namespaces/kubeflow/services/ml-pipeline:8888/proxy', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During development, this allows requests made from the browser that are destined for URLs that would only be available in cluster (ie. /jupyter/
or /pipeline/
) to be proxied to a local Kubernetes proxy server that can be started by running kubectl proxy -p 8083
.
So when you run npm run dev
, the front-end is served on port 8081. Our API server runs on port 8082 with any request made from the front-end to /api
being proxied there. These additional lines allow you to communicate with the other services that will be available at specific top-level routes when deployed.
Here's some more information on what you can do with kubectl proxy
.
https://kubernetes.io/docs/tasks/access-application-cluster/access-cluster/#directly-accessing-the-rest-api
@prodonjs Just verified with the view. One thing is that I don't think that the chart cards scale when the view changes. This is obviously not a relevant issue to this PR, but could perhaps be included if the fix is small enough? |
With respect to the chart resizing issue, I noticed this behavior but I could not figure out how to the responsiveness to work like you would expect despite trying different things in their documentation. The current state at least ensures that the chart takes up the full width of the column, but there doesn't seem to be any detection of when the parent container is resized. I experimented with seeing when the |
I'd vote for the chart staying within it's containing box over responsiveness. |
OK the latest commit should fix the chart display issue. It turns out there was a known bug with Chart.js rendering inside of Polymer Shadow DOM (chartjs/Chart.js#5763). The fix requires injecting some of the Chart.JS css at the component level. https://kf-dev.endpoints.prodonjs-kubeflow-dev.cloud.goog/ has the change deployed which should result in the charts always filling the full size of the container they reside in. |
That's great news! Thanks, @prodonjs |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: avdaredevil 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 |
* Adds Pipelines Cards for definitions and runs. * Address review comments * Fix progress colors and move to common card-styles * Consolidates styles and embeds paper-card inside notebooks and pipelines cards * Adds Chart.JS css to ShadowDOM to enable responsive behavior
* Adds Pipelines Cards for definitions and runs. * Address review comments * Fix progress colors and move to common card-styles * Consolidates styles and embeds paper-card inside notebooks and pipelines cards * Adds Chart.JS css to ShadowDOM to enable responsive behavior
closes #3236
Adds two new cards for showing the 5 most recently created Pipeline definitions and Pipeline runs.
/area front-end
/area centraldashboard
/cc @avdaredevil
/cc @ajayalfred
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)