seqeralabs / nf-tower

Nextflow Tower system
https://tower.nf
Mozilla Public License 2.0
146 stars 52 forks source link

Workflow metrics page #72

Open pditommaso opened 5 years ago

pditommaso commented 5 years ago

The workflow page should include a workflow execution stats metrics similar to the ones produced by the nextflow execution report.

ideally, most of the code should be reused.

pditommaso commented 5 years ago

I've managed to implement a workflow metrics component reusing most of the legacy code from the nextflow execution report.

screencapture-localhost-8000-metrics-2-2019-07-21-15_34_54

For now, it's rendered as an independent page at the url /workflow/metics/:id. Having this it should be straightforward to embed it as a card in the workflow page.

Notably, it uses Plotly JS library imported directly in the component:

https://github.com/seqeralabs/nf-tower/blob/3be2df6ceb365465960e7b578b7ce2273ef6e92b/tower-web/src/app/modules/main/component/workflow-metrics/metrics.component.ts#L17

@tcrespog does this makes the plotly javascript lazy-loaded, or it will be included in the main app bundle once we add this component in the main page?

tcrespog commented 5 years ago

As long as the library isn't included in the package.json, then it shouldn't be eagerly loaded. So, your solution loads the script from the CDN when the workflow page is initialized (therefore, lazily). Why do you want to lazy load this specific library?

tcrespog commented 5 years ago

Oh, now I see that you have added the plotly-1.34.0.min.js inside the component source directory, that seems strange to me. I guess this shouldn't be done if the script is being loaded from the CDN.

pditommaso commented 5 years ago

That was needed to have the class Plotly accessible in the script. Quite neat actually.

I've realised only now that remained the loading from the CDN

https://github.com/seqeralabs/nf-tower/blob/3be2df6ceb365465960e7b578b7ce2273ef6e92b/tower-web/src/app/modules/main/component/workflow-metrics/metrics.component.ts#L36

This was an early tentative, but it was not working randomly likely due to the lazy loading latency. It's supposed to be removed.

pditommaso commented 5 years ago

Reminder to myself, metrics entries display sequence should be consistent with the one displayed in the nextflow execution report. Likely an index attribute needs to be introduced to keep them ordered as they are created.

tcrespog commented 5 years ago

That was needed to have the class Plotly accessible in the script. Quite neat actually.

Ok. Why not loading the dependency from package.json as the usual way?

pditommaso commented 5 years ago

Because it's quite a big javascript file that's only used in that component. I would like to keep the app loading as fast as possible.

tcrespog commented 5 years ago

Ok Paolo. But are you sure this solution achieves the lazy loading behavior that you want? Does it also work in the build form of the application?

pditommaso commented 5 years ago

But are you sure this solution achieves the lazy loading behavior that you want?

No 😄

The main driver here was to reuse the existing code and html layout. The other main thing now is to embed this in the main workflow page showing it only when the workflow execution has completed.

I'll check if it works when building the app in prod mode.

pditommaso commented 5 years ago

umm, I'm seeing this

chunk {0} runtime.465c2333d355155ec5f3.js (runtime) 1.41 kB [entry] [rendered]
chunk {1} main.83d2fdb9d124f70e3f9a.js (main) 2.79 MB [initial] [rendered]
chunk {2} polyfills.c53f45809f78efe47efe.js (polyfills) 43 kB [initial] [rendered]
chunk {3} polyfills-es5.6a9f5fde365a77ab7943.js (polyfills-es5) 69.8 kB [initial] [rendered]
chunk {4} styles.a23b78e918ba472ebd4e.css (styles) 313 kB [initial] [rendered]
chunk {scripts} scripts.eb0ddefb4dc782c7bc0e.js (scripts) 255 kB [entry] [rendered]
Date: 2019-07-22T08:05:13.009Z - Hash: 67e1cac15fba2ddf0084 - Time: 108839ms

WARNING in budgets, maximum exceeded for initial. Budget 2 MB was exceeded by 1.46 MB.

It's warning that the script bundle it's too big

tcrespog commented 5 years ago

:smile: I ask this because as far as I know, I think the default behavior of the import statements is to lazy load the JavaScript modules. In fact, some JS libraries follow a pattern called "tree-shaking" which allows to load just the chosen exported modules without loading the full JS file in case of big libraries (RxJS or Lodash follow this pattern). So we might have to confirm this fact, but I think there wouldn't be any difference between importing the library in this way and including it in the pacakge.json file.

pditommaso commented 5 years ago

Once compiled I don't see the plotly script loaded, therefore it must be included in the main js bundle

Screenshot 2019-07-22 at 10 13 36

I still prefer this import because makes it explicit that the lib is needed by this component.

Then we'll see how to lazy load it. It's not a priority.

tcrespog commented 5 years ago

Ok, I'll leave this here as a future reference: https://github.com/plotly/angular-plotly.js/issues/50

pditommaso commented 4 years ago

Related https://github.com/mohammedzamakhan/ngx-loadable