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

Voxelytics frontend #6460

Merged
merged 26 commits into from
Sep 21, 2022
Merged

Voxelytics frontend #6460

merged 26 commits into from
Sep 21, 2022

Conversation

normanrz
Copy link
Member

@normanrz normanrz commented Sep 12, 2022

Steps to test:

  • Get voxelytics and elasticsearch
  • Set the WORKFLOW_URL env var to http://localhost:9000
  • Set the DEFAULT_WEBKNOSSOS_URL env var to http://localhost:9000
  • Set the WORKFLOW_TOKEN env var to a valid auth token in the above configured webKnossos instance
  • Run vx voxelytics/connect/test/configs/segem_test_2d.yaml --cpu
  • Open http://localhost:9000/workflows

fixes #6468


(Please delete unneeded items, merge only when none are left open)

@normanrz normanrz mentioned this pull request Sep 12, 2022
3 tasks
@normanrz normanrz self-assigned this Sep 16, 2022
@normanrz normanrz marked this pull request as ready for review September 16, 2022 09:18
Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

didn't test yet and only skimmed the new modules, but looks good overall 👍 will try to test now.

frontend/javascripts/libs/polling.ts Outdated Show resolved Hide resolved
@@ -702,3 +703,166 @@ export type ZarrPrivateLink = {
accessToken: string;
expirationDateTime: number | null;
};

export enum VoxelyticsRunState {
Copy link
Member

Choose a reason for hiding this comment

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

If you want to lazy-import a vx bundle, it might be useful to add these types to a new module. In general, we should reorganize some code a bit (e.g., this module currently imports the store which imports the sagas which imports a big part of the code base). However, this is rather out of scope for this PR.

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 think it makes more sense to refactor that in a coordinated effort. If moving these types in a separate file, we at least also need to move the vx-related functions from admin_rest_api.ts to a separate file.

@philippotto
Copy link
Member

a comparison of the old and new bundle sizes would be interesting before we merge this.

@normanrz
Copy link
Member Author

a comparison of the old and new bundle sizes would be interesting before we merge this.

6.6M vs 7.2M uncompressed (calculated with yarn build && du -csh public/bundle/*.{js,css})

Bundle splitting turns out to be quite difficult :-/

@philippotto
Copy link
Member

philippotto commented Sep 21, 2022

a comparison of the old and new bundle sizes would be interesting before we merge this.

6.6M vs 7.2M uncompressed (calculated with yarn build && du -csh public/bundle/*.{js,css})
Bundle splitting turns out to be quite difficult :-/

I had a look at the devtools on https://master.webknossos.xyz vs https://vxfrontend.webknossos.xyz/ and there the difference is 381 kB vs. 380 kB for main.js (so, basically nothing) and 1.1 MB vs. 966 kB for vendors~main.js. So, this means bundle splitting would probably only be beneficial to separate the new vendors code into a new module. Is this also difficult? Either way, it's probably not really necessary, right now (given the numbers). Still, it would be good to know whether further vendor splitting might be doable without too much effort.

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

🍭

@normanrz
Copy link
Member Author

Still, it would be good to know whether further vendor splitting might be doable without too much effort.

It could be done, but we'd need to rework the code splitting config in webpack. We need to figure out how to assign stable names to the individual bundles (currently, hard-coded to vendors~main) or figure out dynamic imports in our system (currently, the vendors bundle is statically imported via HTML script tags).

@normanrz normanrz merged commit 7471fa3 into vx-routes Sep 21, 2022
@normanrz normanrz deleted the vx-frontend branch September 21, 2022 12:24
bulldozer-boy bot pushed a commit that referenced this pull request Sep 22, 2022
* adds log routes
* fixes log routes
* fixes log routes
* moar routes
* Create unified json format for ObjectId
* remove unused imports
* fixes
* works
* Merge branch 'master' into vx-routes
* schema
* schema
* lint
* refactoring
* Merge branch 'master' into objectid-json
* pr feedback
* merge
* pr feedback
* Merge remote-tracking branch 'origin/master' into vx-routes
* rename tables
* fixes
* fixes
* messages and conf
* Merge remote-tracking branch 'origin/master' into vx-routes
* fixes Elasticsearch polling and setup
* lint
* remove @Api annotation from VoxelyticsController
* Merge branch 'master' into vx-routes
* pr feedback
* update snapshots
* pr feedback
* pr feedback
* revert blanket-cors header
* no cors in vx routes
* Merge remote-tracking branch 'origin/master' into vx-routes
* fixes log levels
* pr feedback
* format
* move
* changelog
* Update app/models/voxelytics/VoxelyticsLogLevel.scala

Co-authored-by: Florian M <[email protected]>
* Merge branch 'master' into vx-routes
* Voxelytics frontend (#6460)

* wip voxelytics workflow viewer frontend

* fixes JS code

* stuff

* styling

* expose features.voxelyticsEnabled

* revert changes to Application.scala

* format

* pr feedback

* output ES2020

* styling

* styling

Co-authored-by: Florian M <[email protected]>
* styling
* merge
* evolutions
* Merge branch 'master' into vx-routes
* Merge branch 'master' into vx-routes
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

Successfully merging this pull request may close these issues.

3 participants