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

[bug] Mangling breaks the code #364

Closed
sdll opened this issue Jun 6, 2019 · 17 comments
Closed

[bug] Mangling breaks the code #364

sdll opened this issue Jun 6, 2019 · 17 comments

Comments

@sdll
Copy link

@sdll sdll commented Jun 6, 2019

Bug report

Version

rollup-plugin-terser@5.0.0

minify() options used

https://github.com/tensorflow/tfjs-models/blob/8f9a0e1de5ebf74742e6f5ad59d0f9c8704bfe06/deeplab/rollup.config.js#L44

terser input

See tensorflow/tfjs-models#229.

terser error

When mangling is enabled, running the demo either misses object properties, or fails to find an imported function (e.g. tf.tidy, mangled as n). With mangling disabled, the code works fine.

Could you please tell me what might be the cause of the problem?

@sdll sdll mentioned this issue Jun 6, 2019
9 of 9 tasks complete
@fabiosantoscode

This comment has been minimized.

Copy link
Collaborator

@fabiosantoscode fabiosantoscode commented Jun 7, 2019

Hello there! Please try to reproduce the issue with a small code example so we can get to it.

@TrySound

This comment has been minimized.

Copy link
Contributor

@TrySound TrySound commented Jun 7, 2019

@sdll Does disabling collapse_vars solve your problem?

@sdll

This comment has been minimized.

Copy link
Author

@sdll sdll commented Jun 7, 2019

@fabiosantoscode, I will get back when I have more bandwidth.

@TrySound, thank you for the suggestion! Everything works well with compress set to true, problems arise only when mangling is enabled. When collapse_vars is disabled, but mangling is enabled, the demo still fails with TypeError: (0 , t.tidy) is not a function. I could not find (0 , t.tidy) in the minified deeplab.esm.js, does terser minify node_modules as well by any chance?

@fabiosantoscode

This comment has been minimized.

Copy link
Collaborator

@fabiosantoscode fabiosantoscode commented Jun 7, 2019

@sdll also have you tried disabling the module:true option? I don't remember whether rollup-plugin-terser minifies every little module or if it minifies the whole thing in the end. If it's the former, you need module:true. If it's the latter, you need module:true in your ejs bundle and module:false in your CJS bundle.

@sdll

This comment has been minimized.

Copy link
Author

@sdll sdll commented Jun 7, 2019

Alas, @fabiosantoscode, module: false does not fix the issue, the demo fails with the same trace. Thank you for the support!

@fabiosantoscode

This comment has been minimized.

Copy link
Collaborator

@fabiosantoscode fabiosantoscode commented Jun 7, 2019

Also Terser does not minify or know about node_modules.

@TrySound

This comment has been minimized.

Copy link
Contributor

@TrySound TrySound commented Jun 7, 2019

Plugin runs terser for each chunk

@fabiosantoscode

This comment has been minimized.

Copy link
Collaborator

@fabiosantoscode fabiosantoscode commented Jun 29, 2019

TL;DR: possible workarounds

  1. Use mangle: { reserved: ['tf'] } to ensure tf is never mangled.
  2. Put the rollup-plugin-node-resolve plugin after the terser plugin (not sure if this will work)

@sdll in your rollup config, there's a line that says that imports to @tensorflow/tfjs should be redirected to the global variable tf.

https://github.com/tensorflow/tfjs-models/pull/229/files#diff-52a7421250b57b207b930d54490cef68R72

Check out more on how the ouptut.globals option works: https://rollupjs.org/guide/en#output-globals

There might be a crazy interaction between one or more of the plugins you're using and the globals option, but I don't believe this could be a Terser bug per se. Variables whose declarations don't appear in the JS source are treated as "free variables", therefore possibly global and possibly an error because the variable is undefined. This hasn't changed since UglifyJS has been forked. I'm sure that imported symbols are properly marked as in-scope, as I've implemented it myself years ago for uglify-es when it was just a separate branch on the UglifyJS repository.

Let me know if one of the workarounds works so we can get to the bottom of this issue and inform future googlers of what might be happening here 🔥 🔥 🔥

@sdll

This comment has been minimized.

Copy link
Author

@sdll sdll commented Jun 29, 2019

@fabiosantoscode, thanks for looking into the issue!

  1. Adding mangle: { reserved: ['tf'] } did the trick, and the demo rolls smooth.
  2. Reordering the plugins to load rollup-plugin-node-resolve last by itself did not fix the problem.
  3. Doing both works as well.

I will stick to reserving tf, thank you so much for your help and time!

@fabiosantoscode

This comment has been minimized.

Copy link
Collaborator

@fabiosantoscode fabiosantoscode commented Jun 30, 2019

@TrySound do you think this might be an issue on Rollup's end or just misconfiguration?

@TrySound

This comment has been minimized.

Copy link
Contributor

@TrySound TrySound commented Jun 30, 2019

@sdll Could you provide gist with minified output?

@fabiosantoscode Not sure yet.

@sdll

This comment has been minimized.

Copy link
Author

@sdll sdll commented Jul 1, 2019

@TrySound, do you mean the stack trace with which the demo fails?

@TrySound

This comment has been minimized.

Copy link
Contributor

@TrySound TrySound commented Jul 1, 2019

This error is runtime, right? I need to see the code which is fails.

@sdll

This comment has been minimized.

Copy link
Author

@sdll sdll commented Jul 1, 2019

@TrySound, correct, the demo fails with the error described above if mangling is set to true.

Working example:

https://gist.github.com/sdll/1045f53a48856a6b7f17106b144f6c7f

Failing example:

https://gist.github.com/sdll/0054bc4248207654779e7bdf94ff8e88

If you want to run the demo yourself, first do yarn publish-local in the project root directory, then cd into demo, run yarn link-local and yarn watch.

@TrySound

This comment has been minimized.

Copy link
Contributor

@TrySound TrySound commented Jul 1, 2019

You sure they are not the same?

@sdll

This comment has been minimized.

Copy link
Author

@sdll sdll commented Jul 1, 2019

@TrySound, my bad. Fixed now.

dsmilkov added a commit to tensorflow/tfjs-models that referenced this issue Jul 29, 2019
This PR adds the DeepLab model together with a demo.

~~There are several issues with the implementation at the moment.~~ Here is the todo list:

- [x] **Fix the grayscale-only segmentation maps**

~~The culprit might be [here](https://github.com/tensorflow/tfjs-models/blob/5fa0787a968799f0a614c663f83afb09b7e8f2cb/deeplab/src/utils.ts#L86).~~

**UPDATE 05/06**: The problem was resolved by constructing a single buffer for the translated segmentation map, instead of building the tensor by stacking three separate channel buffers (see [here](218383a)). I do not quite understand why this solves the problem.

- [x] **Show performance stats in the demo**

- [x] **Add loader for improved UX**

- [x] **~~Fix~~ ~~Disable~~ Reserve `tf` to make mangling work**

The code uses ES6 features, so I have picked [`rollup-plugin-terser`](https://github.com/TrySound/rollup-plugin-terser) instead of uglify, which breaks on the compilation of the esm module. ~~Terser, however, is pestered with bugs when mangling is enabled: the demo fails to recognise the minified `tf.tidy` function (the error is, say, `t.tidy is not a function`). I might as well revert to uglify after refactoring the code.~~

**UPDATE 06/06**: ~~The problem persists even after playing with the mangling options, with the model missing `predict` method in the debugging mode. I have disabled mangling altogether and reported the issue (terser/terser#364

**UPDATE 29/06**: Mangling works well when `tf` is reserved. See the awesome [explanation](terser/terser#364 (comment)) by @fabiosantoscode why this might be the case.

- [x] **Improve docs**

- [x] **Add tests**

- [x] **Explore quantisation**

**UPDATE 07/06**: The models are quantized by default.

- [x] Add Cityscapes and ADE20K Labelling Schemes
- [x] Change the gif demo to show all models with updated color mappings

Please let me know if I am missing anything.

cc: @manrajgrover
@fabiosantoscode

This comment has been minimized.

Copy link
Collaborator

@fabiosantoscode fabiosantoscode commented Sep 25, 2019

Closing this due to radio silence. Tag me if a minimal reproduction comes up please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.