Skip to content

Add --graph-only option for the orca serve entry point #112

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
wants to merge 1 commit into from

Conversation

jonmmease
Copy link
Contributor

Overview

Added a --graph-only boolean command line option to the orca serve entry point. When this option is present, only the plotly-graph component is run (not thumbnail, dash, etc.).

When graph conversion is the only service needed, this option reduces the number of electron processes, the memory usage, and startup time.

Past Discussion

See #110

Resource usage comparison

Here's a comparison of the baseline resource usage on OSX

Without --graph-only (legacy behavior)

screen shot 2018-08-09 at 8 15 23 am

There are 8 processes running, consuming ~390MB of memory.

Based on my testing from Python, from launching the server to receiving the first image conversion takes about 2.2 seconds.

With `--graph-only

screen shot 2018-08-09 at 8 15 59 am

There are 3 processes running, consuming ~120MB of memory.

Based on my testing from Python, from launching the server to receiving the first image conversion takes about 1.7 seconds.

Resource Summary

If all you need is graph conversion, this flag saves 5 processes, 270MB of memory, and a half second of startup time.

Testing

I added a new integration testing file at test/integration/orca_serve_graph-only_test.js. This file is based on a subset of the testing in test/integration/orca_serve_test.js. But it checks to make sure there is only 1 window running, that the graph-component is running, and that the thumbnail component is not running.

When this option is present, only the plotly-graph component is run
(not thumbnail, dash, etc.).

When graph conversion is the only service needed, this option
reduces the number of electron processes, the memory usage,
and startup time.
@jonmmease jonmmease changed the title Add --graph-only option for the orca serve entry point Add --graph-only option for the orca serve entry point Aug 9, 2018
@etpinard
Copy link
Contributor

etpinard commented Aug 9, 2018

Brilliant PR @jonmmease ! Thanks very much!

Now, that docker-build-and-push job is failing again, and but the looks of it it never ran successfully off one of your branches.

Digging deeper, the environment variable $DOCKER_USER and $DOCKER_PASS are blank on runs off your branches whereas they are correctly filled otherwise.

So, I think we should probably disable docker-build-and-push off forks. I hope there's a way to do that in the circle config. I doubt we want to pass environment variable to all open-source contributors. Alternatively, @jonmmease you could start branching off the official repo (I invited you to join as collaborator) instead of off your fork.

@jonmmease
Copy link
Contributor Author

Sure thing!

Yeah, it would be nice to disable the docker-build-and-push CI job for forks if there's a way. But in the meantime I'll go ahead and move my two current PRs to the main repo and see if that takes care of it.

Copy link
Contributor

@etpinard etpinard left a comment

Choose a reason for hiding this comment

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

But in the meantime I'll go ahead and move my two current PRs to the main repo and see if that takes care of it.

Thanks 🎉

name: 'graph-only',
type: 'boolean',
alias: ['graphOnly'],
description: 'Launches only the graph component (not thumbnails, dash, etc.) to save memory and reduce the number of processes'
Copy link
Contributor

Choose a reason for hiding this comment

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

🐄 . at the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in #114 🙂

@jonmmease jonmmease mentioned this pull request Aug 9, 2018
@jonmmease
Copy link
Contributor Author

Replaced by #114

@jonmmease jonmmease closed this Aug 9, 2018
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.

2 participants