Remove Tone.js from JavaScript package?

[teropa]

The package at https://cdn.jsdelivr.net/npm/@magenta/music@0.0.5/dist/magentamusic.min.js now includes Tone.js as well, as the player using it is exported from core.ts.

I don't think this is something users would expect, and may cause issues with projects using other versions of Tone.

[adarob]

I like having the player accessible from our package. Is there any way to make this work?

On Wed, Apr 25, 2018, 10:02 PM Tero Parviainen notifications@github.com wrote:

The package at https://cdn.jsdelivr.net/npm/@magenta/music@0.0.5/dist/magentamusic.min.js now includes Tone.js as well, as the player using it is exported from core.ts.

I don't think this is something users would expect, and may cause issues with projects using other versions of Tone.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/tensorflow/magenta-js/issues/56, or mute the thread https://github.com/notifications/unsubscribe-auth/ABCa6O05BPK7bEavyKT_D40Vj6b88cNcks5tsVTsgaJpZM4TkhzP .

[adarob]

@cghawthorne

On Wed, Apr 25, 2018, 10:27 PM Adam Roberts adarob@google.com wrote:

I like having the player accessible from our package. Is there any way to make this work?

On Wed, Apr 25, 2018, 10:02 PM Tero Parviainen notifications@github.com wrote:

The package at https://cdn.jsdelivr.net/npm/@magenta/music@0.0.5/dist/magentamusic.min.js now includes Tone.js as well, as the player using it is exported from core.ts.

I don't think this is something users would expect, and may cause issues with projects using other versions of Tone.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/tensorflow/magenta-js/issues/56, or mute the thread https://github.com/notifications/unsubscribe-auth/ABCa6O05BPK7bEavyKT_D40Vj6b88cNcks5tsVTsgaJpZM4TkhzP .

[teropa]

Should still be possible by using an NPM peer dependency, and possibly configuring Browserify so that it doesn't include Tone but just expects it to be around.

2018-04-26 5:27 GMT+00:00 Adam Roberts notifications@github.com:

@cghawthorne

On Wed, Apr 25, 2018, 10:27 PM Adam Roberts adarob@google.com wrote:

I like having the player accessible from our package. Is there any way to make this work?

On Wed, Apr 25, 2018, 10:02 PM Tero Parviainen <notifications@github.com

wrote:

The package at https://cdn.jsdelivr.net/npm/@magenta/music@0.0.5/dist/ magentamusic.min.js now includes Tone.js as well, as the player using it is exported from core.ts.

I don't think this is something users would expect, and may cause issues with projects using other versions of Tone.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/tensorflow/magenta-js/issues/56, or mute the thread https://github.com/notifications/unsubscribe-auth/ABCa6O05BPK7bEavyKT_ D40Vj6b88cNcks5tsVTsgaJpZM4TkhzP .

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tensorflow/magenta-js/issues/56#issuecomment-384517854, or mute the thread https://github.com/notifications/unsubscribe-auth/AAC7YOEZ2y2Yf39lcIbdpwjt6ldAGzNiks5tsVrdgaJpZM4TkhzP .

[adarob]

But that would make this example no longer work, correct? https://github.com/tensorflow/magenta-js/blob/master/music/README.md#getting-started

[adarob]

Yeah, I think we'll need something like gulp to do that: https://9elements.com/io/external-bundles-with-browserify-and-gulp/

[adarob]

@tambien do you have a suggestion for handling this?

[tambien]

i'm actually working on this myself. Tone.js currently doesn't play too nice when it's loaded multiple times on a page. i was talking to someone yesterday about this and may have a solution to the problem. i think it could be as simple as setting a browser global so that all versions can access the same AudioContext.

[adarob]

I was also looking into leaving it out of our bundle and expecting it to loaded separately if our player is used. However it seems that tone is not found by my bundle even if I load the tone bundle first. If I browserify my own bundle with only tone in it and load that first, it does work. Any ideas?

On Thu, Apr 26, 2018, 7:30 AM Yotam Mann notifications@github.com wrote:

i'm actually working on this myself. Tone.js currently doesn't play too nice when it's loaded multiple times on a page. i was talking to someone yesterday about this and may have a solution to the problem. i think it could be as simple as setting a browser global so that all versions can access the same AudioContext.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/tensorflow/magenta-js/issues/56#issuecomment-384662049, or mute the thread https://github.com/notifications/unsubscribe-auth/ABCa6NLnZnOGSFVE2-h2PDL9NFylDJvbks5tsdojgaJpZM4TkhzP .

[adarob]

I think I know how to do it now. I'll give it a try this afternoon.

On Thu, Apr 26, 2018, 8:03 AM Adam Roberts adarob@google.com wrote:

I was also looking into leaving it out of our bundle and expecting it to loaded separately if our player is used. However it seems that tone is not found by my bundle even if I load the tone bundle first. If I browserify my own bundle with only tone in it and load that first, it does work. Any ideas?

On Thu, Apr 26, 2018, 7:30 AM Yotam Mann notifications@github.com wrote:

i'm actually working on this myself. Tone.js currently doesn't play too nice when it's loaded multiple times on a page. i was talking to someone yesterday about this and may have a solution to the problem. i think it could be as simple as setting a browser global so that all versions can access the same AudioContext.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/tensorflow/magenta-js/issues/56#issuecomment-384662049, or mute the thread https://github.com/notifications/unsubscribe-auth/ABCa6NLnZnOGSFVE2-h2PDL9NFylDJvbks5tsdojgaJpZM4TkhzP .

[tambien]

@teropa i've got a question, is the reason that you don't want Tone.js packaged into magenta is bc you're using Tone.js in the rest of your app and that'll cause two versions of Tone.js to be loaded?

talking with @adarob about this, i think i can add a global to Tone.js to make it so that multiple instances running at the same time share the same AudioContext and so they play nice with each other. it will obviously add some to the package size and also log Tone.js rX multiple times in the console... but i'm thinking that might be better for versioning and ease of install than a peerDep.

[teropa]

@tambien Yep, exactly. And I imagine a lot of people will be doing this. If the versions won't step on each other's toes then should be fine.

@adarob Ah, hadn't realized the player would be for main package inclusion too, that changes things a bit. :)

[tambien]

@teropa ok good to understand what the objective is. i'm going to try something with this next release that will hopefully accommodate this use case. i think you're right that there will be a number of people trying to include tone on a project like this which already includes tone as a dependency. i had this same issue with the Piano and i never quite got a solution i was happy with.

[adarob]

Cool, so the plan is to leave the package as-is for now and weight until @tambien can make changes to tone. @teropa, is this blocking you from updating your codepens?

[starakaj]

Hey all, first of all thanks for all the amazing work here. I'm really excited to start playing around with the code and to try to get Magenta running in some new contexts.

I'm also running into an issue with Tone in the package, though it has less to do with conflicting versions of Tone, and more to do with getting Magenta working outside of a browser. Right now I'm trying to get the drum rnn example from this codepen https://codepen.io/teropa/pen/JLjXGK running in Node. As soon as I get this far:

import * as mm from "@magenta/music";
const rnn = new mm.MusicRNN(
    'https://storage.googleapis.com/download.magenta.tensorflow.org/tfjs_checkpoints/music_rnn/drum_kit_rnn'
);
rnn.initialize();

and try to run everything through the typescript compiler, there's a Tone error when trying to build the module.

/Users/starakaj/git/live-tensor/node_modules/tone/build/Tone.js:633
                var hasAudioContext = window.hasOwnProperty('AudioContext') || window.hasOwnProperty('webkitAudioContext');
                                      ^

ReferenceError: window is not defined
    at Function.get (/Users/starakaj/git/live-tensor/node_modules/tone/build/Tone.js:633:36)
    at /Users/starakaj/git/live-tensor/node_modules/tone/build/Tone.js:1179:15
    at Module (/Users/starakaj/git/live-tensor/node_modules/tone/build/Tone.js:25:3)
    at /Users/starakaj/git/live-tensor/node_modules/tone/build/Tone.js:1178:2
    at Tone (/Users/starakaj/git/live-tensor/node_modules/tone/build/Tone.js:9:20)
    at Object.<anonymous> (/Users/starakaj/git/live-tensor/node_modules/tone/build/Tone.js:14:2)
    at Module._compile (module.js:652:30)
    at Object.Module._extensions..js (module.js:663:10)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)

Makes sense, as of course there's no window. But all I'm trying to do is get the RNN—no real reason to get Tone involved. Sorry if there's an easy way around this I haven't thought of, and thanks in advance for any ideas.

[mattetti]

I run into the same issue, I hacked tone.js to bypass the issue but it would be nice if the core classes weren't depending on the browser

[adarob]

@tambien is there a better solution for this now?

[mattetti]

@adarob that was actually fixed in a newer version of tone.js https://github.com/Tonejs/Tone.js/blob/c023181579d55bac110b9d369f79aa5acd275499/CHANGELOG.md#1349

Changing references to window allowing it to not throw error in node context

Unfortunately there seem to be some issues upgrading to the latest version as per #294

I can't promise anything but I might going to try to upgrade tone as a way to learn more about the internals of this project. I might get stuck quickly but I'll give it a try.

[JakobFelixJulius]

Hello! I am also interested in a version without the Tone.js dependency, trying to get magenta.js working on a node server (since some heavy generating tasks block the rest of the web app). Anybody know any workaround or solution for this?

[tambien]

@jakobsudau i've gotten around this problem in the past by just deep-requiring the individual resources that i need. but the references to the window and AudioContext should be fixed when the upgraded version of Tone is merged.

[JakobFelixJulius]

@tambien thanks for your reply! I will look into that and hope that your pull request #316 will get merged soon 😊

[teropa]

With tensorflow/tfjs-core#1771 it should soon be possible to run TensorFlow.js in Web Workers, also allowing Magenta.js models to run off the UI thread, which'll be great for interactive applications.

This'll be easier to do if there's a library free of dependencies to any AudioContext or UI stuff. Perhaps separate "core" and "full" packages would be the way to go? The full being the current batteries-included package, and the core the bare minimum needed to run the models.

[mattetti]

@teropa I agree, and ideally we could provide some sort of interface/adapters so one can bring their own player/lower level audio implementation.

[teropa]

We're successfully running Magenta models in Web Workers with a custom library build that has the following changes applied:

• Excluding ./player, ./recorder, and ./visualizer from the exports. This leaves out Tone and UI code.
• Lazily initialising AudioContext in audio_utils.ts. AudioContexts are not available in workers so if we try to initialize one on the top level, the library won't load. Also notably, models that need the context (such as transcription) won't function in the worker.
• In lib.ts, plopping the global mm onto self, not window.

Happy to turn this into a contribution that would build an additional "core" library with the aforementioned changes?

[notwaldorf]

Honestly, my dream here is to not vend another monolithic music library, but have it split in small ones (we started towards this when we added webpack and never got far)

Like, you should be able to use a magenta model without also having to download Tone, the players, the other models, etc. I think several build artifacts might be a little out of scope right now since it’s most likely going to be a breaking change and take quite a bit of work.

@adarob what are your thoughts on this?

On Tue, Jul 23, 2019 at 12:07 AM Tero Parviainen notifications@github.com wrote:

We're successfully running Magenta models in Web Workers with a custom library build that has the following changes applied:

• Excluding ./player, ./recorder, and ./visualizer from the exports. This leaves out Tone and UI code.
• Lazily initialising AudioContext in audio_utils.ts. AudioContexts are not available in workers so if we try to initialize one on the top level, the library won't load. Also notably, models that need the context (such as transcription) won't function in the worker.
• In lib.ts, plopping the global mm onto self, not window.

Happy to turn this into a contribution that would build an additional "core" library with the aforementioned changes?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/tensorflow/magenta-js/issues/56?email_source=notifications&email_token=AAKOIUUN33N2KUJ7ZQ2W6GDQA2U4HA5CNFSM4E4SDTH2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2SEPEA#issuecomment-514082704, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKOIUUQCGAO32V46YXPGFDQA2U4HANCNFSM4E4SDTHQ .

[tambien]

Haven't done a ton of research into this, but shouldn't tree-shaking remove the need for splitting apart @magenta/music into multiple packages?

In an ideal world import { Coconet } from @magneta/music would only pull in just the model and it's dependency files, so if that model didn't rely on anything with Web Audio / Tone.js, then it wouldn't be compiled into your code. In practice i've found that you've got to deep require Coconet by doing something like import {Coconet} from @magenta/music/es5/coconet/Model.

Reading a little about tree-shaking with typescript just now, seems like maybe setting the tsconfig to export an esnext module with import/export statements instead of all require statements would enable tree-shaking in compilers that support it (like webpack).

I haven't tested this out myself, just wondering if this might be a simpler direction that is better setup for future browser/ES6 developments vs reorganizing the codebase into multiple packages.

[teropa]

I agree tree-shaking should take care of this for any app that uses a bundler.

My use case was to have something to load into a Worker with importScripts, and that required us to do a custom build right now.

[notwaldorf]

Alright so I've been looking into this, and we ship the exploded build to npm (https://www.jsdelivr.com/package/npm/@magenta/music?path=es5). Has anyone tried using an import for just the model they want (sans Tone.js), for example (something like const mvae = require("@magenta/music/es5/music_vae");?

I know it's in es5 and that sucks; working on publishing the exploded build to es6 as well.