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

Jump to bottom

Fix lab build prod #6907

Merged
merged 4 commits into from Aug 1, 2019
Merged

Fix lab build prod #6907

merged 4 commits into from Aug 1, 2019

Conversation

telamonian
Copy link
Member

Quoting @tibdex from #6839:

References

Code changes

The changes are small an explained in comments and in the commit messages.

User-facing changes

End-users will have a better UX since they will be running optimized code.

Backwards-incompatible changes

There should be none as switching to prod build by default is non breaking and Webpack is an implementation detail.

@jupyterlab-dev-mode
Copy link

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@telamonian
Copy link
Member Author

I added a couple of commits to this PR that:

  • add minimize as an option for lab build.
  • defaults the lab build to dev if there's anything in the "linked_packages" or "local_extensions" fields of build_config.json, and to prod otherwise.

@telamonian
Copy link
Member Author

telamonian commented Jul 26, 2019 •
edited

Quoting @blink1073:

After a rebase I think the logic looks good. Let’s wait until master is at 1.1dev to merge since this is a cli change

@telamonian telamonian mentioned this pull request Jul 26, 2019
Closed
@telamonian
Copy link
Member Author

Rebase is done, this is ready to go when 1.1dev is created.

blink1073
blink1073 approved these changes Jul 26, 2019
View reviewed changes
Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Thanks!

vidartf
vidartf reviewed Jul 26, 2019
View reviewed changes
yarn.lock Outdated Show resolved Hide resolved
tibdex and others added 3 commits July 30, 2019 21:48
@tibdex @telamonian
Fix production build
c1a5803 
With the `TerserPlugin` enabled, the build fails with:

```
~/.local/share/virtualenvs/project-QesvCi_o/share/jupyter/lab/staging
❯ node yarn.js webpack --config webpack.prod.config.js --progress
yarn run v1.15.2
$ /Users/me/.local/share/virtualenvs/project-QesvCi_o/share/jupyter/lab/staging/node_modules/.bin/webpack --config webpack.prod.config.js --progress
 97% [0] chunk asset optimization TerserPlugin
<--- Last few GCs --->

[88233:0x108000000]    26766 ms: Scavenge 1392.5 (1420.9) -> 1391.9 (1421.9) MB, 2.2 / 0.0 ms  (average mu = 0.067, current mu = 0.029) allocation failure

<--- JS stacktrace --->

==== JS stack trace =========================================

    0: ExitFrame [pc: 0x2b3e51fdbe3d]
Security context: 0x1b222981e6e9 <JSObject>
    1: _walk [0x1b2248a66fc1] [/Users/me/.local/share/virtualenvs/project-QesvCi_o/share/jupyter/lab/staging/node_modules/terser/dist/bundle.min.js:~1] [pc=0x2b3e5223920d](this=0x1b225aef3559 <AST_String map = 0x1b22fdfd8419>,e=0x1b22c00c09c1 <En map = 0x1b22fdfe44f1>)
    2: /* anonymous */ [0x1b22d2ca1001] [/Users/me/.local/share/virtualenvs/chouke...

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory
 1: 0x10003cf99 node::Abort() [/Users/me/.asdf/installs/nodejs/10.16.0/bin/node]
 2: 0x10003d1a3 node::OnFatalError(char const*, char const*) [/Users/me/.asdf/installs/nodejs/10.16.0/bin/node]
 3: 0x1001b7835 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, bool) [/Users/me/.asdf/installs/nodejs/10.16.0/bin/node]
 4: 0x100585682 v8::internal::Heap::FatalProcessOutOfMemory(char const*) [/Users/me/.asdf/installs/nodejs/10.16.0/bin/node]
 5: 0x100588155 v8::internal::Heap::CheckIneffectiveMarkCompact(unsigned long, double) [/Users/me/.asdf/installs/nodejs/10.16.0/bin/node]
 6: 0x100583fff v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector, v8::GCCallbackFlags) [/Users/me/.asdf/installs/nodejs/10.16.0/bin/node]
 7: 0x1005821d4 v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace, v8::internal::GarbageCollectionReason, v8::GCCallbackFlags) [/Users/me/.asdf/installs/nodejs/10.16.0/bin/node]
 8: 0x10058ea6c v8::internal::Heap::AllocateRawWithLigthRetry(int, v8::internal::AllocationSpace, v8::internal::AllocationAlignment) [/Users/me/.asdf/installs/nodejs/10.16.0/bin/node]
 9: 0x10058eaef v8::internal::Heap::AllocateRawWithRetryOrFail(int, v8::internal::AllocationSpace, v8::internal::AllocationAlignment) [/Users/me/.asdf/installs/nodejs/10.16.0/bin/node]
10: 0x10055e434 v8::internal::Factory::NewFillerObject(int, bool, v8::internal::AllocationSpace) [/Users/me/.asdf/installs/nodejs/10.16.0/bin/node]
11: 0x1007e6714 v8::internal::Runtime_AllocateInNewSpace(int, v8::internal::Object**, v8::internal::Isolate*) [/Users/me/.asdf/installs/nodejs/10.16.0/bin/node]
12: 0x2b3e51fdbe3d
13: 0x2b3e5223920d
14: 0x2b3e51f918d5
15: 0x2b3e5223a1f7
```

Even on powerful laptops such as a MacBook Pro with a 2,3 GHz Intel Core i9 and 32 GB 2400 MHz DDR4.

The Node.js process would never exit and thus commands like:

```
jupyter labextension install @jupyterlab/plotly-extension --dev-build False
```

would never exit too but without telling why (see jupyterlab#4276).

I've tried a bunch of different terser options but I could not find a combination that would succeed when installing relatively big JupyterLab extensions.
So I resort to disabling minification altogether. I think it's better to have a fat production build than no production build at all.
@tibdex @telamonian
Default to production build
e70ce00 
Now that the production build is fixed, I think it's better to default to it. Indeed, we want users installing JupyterLab to have the best experience using the interface and it means running the optimized production build.

This is especially true for extensions built with React because in dev mode, they will use the React's development bundle which is known to be much slower than the production one.

See also: jupyterlab#6804 (comment)
@telamonian
added explicit minimize option to lab build
7e69eeb
@telamonian
jupyter lab build defaults to dev if any linked_packages or local_e…
f7f3fb8 
…xtensions

defaults to prod build otherwise
@jasongrout
Copy link
Contributor

@blink1073 - merge if you think this is ready to be merged

@blink1073
Copy link
Member

In it goes!

@blink1073 blink1073 merged commit bdf6136 into jupyterlab:master Aug 1, 2019
This was referenced Aug 2, 2019
Closed
Closed
@telamonian telamonian mentioned this pull request Aug 15, 2019
Closed
@jasongrout
Copy link
Contributor

Should we backport this to 1.0.x? In the end, did it break any apis that someone may be using developing an extension?

@blink1073
Copy link
Member

We changed the default CLI behavior, which could break some builds.

@jasongrout
Copy link
Contributor

So it's a breaking change, as in 2.0? How would a build break?

@blink1073
Copy link
Member

Running out of memory because it would now use the Uglify plugin.

@telamonian
Copy link
Member Author

There are definitely users who will have to adjust their build commands. For example, this guy is an example of user who would probably now need to do jupyter lab build --minimize=False.

It seems that the users who will run into trouble are those with the special circumstance of trying to build Jlab in a resource-constrained VM. Currently, they're essentially relying on an unintended behavior of jupyter lab build (it will always do a dev build, even for end users, and the dev build just so happens to not use the memory-intensive minifier).

However, unlike a breaking change to an API, there will be an easy way for the affected end users to fix their own problem (ie by adding the --minimize=False arg). So I think the best solution would be to add an informative message when the build fails. The message would explain the --dev-build and --minimize args, and also explain how to add them to their config files in order to ensure that their builds always work correctly.

I doubt that we can reliably catch all flavors of out of memory errors, so we could maybe just have that message displayed whenever the build fails for whatever reason.

@vidartf
Copy link
Member

vidartf commented Aug 16, 2019

Running out of memory because it would now use the Uglify plugin.

*Terser plugin, right?

@blink1073
Copy link
Member

👍 for a backport after adding the informative message.

*Terser plugin, right?

Yeah, that one 😉

@telamonian telamonian mentioned this pull request Aug 16, 2019
Merged
@telamonian
Copy link
Member Author

I took a stab at adding the troubleshooting message in #7037. The PR probably isn't ready yet, and could definitely use some feedback.

@blink1073 blink1073 added this to the 1.1 milestone Aug 22, 2019
@lock lock bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Sep 21, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Sep 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vidartf vidartf vidartf left review comments

@blink1073 blink1073 blink1073 approved these changes

Assignees

@blink1073 blink1073

Labels
status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Milestone
1.1
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@telamonian @jasongrout @blink1073 @vidartf @tibdex