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

Loading pre-trained models #446

Closed
Nacho114 opened this issue Aug 8, 2022 · 2 comments · Fixed by #456
Closed

Loading pre-trained models #446

Nacho114 opened this issue Aug 8, 2022 · 2 comments · Fixed by #456
Labels
bug Something isn't working discojs Related to Disco.js server Related to the server
Milestone

Comments

@Nacho114
Copy link
Contributor

Nacho114 commented Aug 8, 2022

Due to issues with the TFJS back end, both loading and saving (#363) models from/to disk is currently not possible in discojs. In particular this means we cannot use pre-trained models.

Currently models are built and then said models are made available in the server.

If we have a pre trained model stored as a model.json + weights.bin, we would load it as follows:

await tf.loadLayersModel('file:./modelPath/model.json')

This however yields the following error:

[Tensorflow Node-js, TypeError: Only HTTP(S) protocols are supported](https://stackoverflow.com/questions/60137483/tensorflow-node-js-typeerror-only-https-protocols-are-supported)

As with #363, using both tfjs and tfjs-node seems to cause unexpected behavior as reported here

@tharvik Suggested to import either tfjs or tfjs-node on discojs depending on if it is used by browser or node, since the api is identical this should work in principle. Perhaps a simpler solution is possible.

Another idea ( @s314cy ) is to refactor the pertinent code from discojs back into the server.

@Nacho114 Nacho114 added bug Something isn't working discojs Related to Disco.js server Related to the server labels Aug 8, 2022
@Nacho114
Copy link
Contributor Author

Nacho114 commented Aug 10, 2022

Issues with approach 2 (refactoring code back to the serve)

Using the easier approach of refactoring the pertinent code from discojs back into the server yields a previous error #379, so we have come full circle. To summarize the issue

Say model.ts is the file that contains the script of loading models, currently this is in discojs, it used to be in the server.

Issues server side

If model.ts is in the server, we get #379 in the benchmark (and sometimes even in the UI? when going between training and testing). Somehow the tfjs engine has issues keeping track of variable names in the asynchronous js (await style code), and so accidentally sets the same variables with the same name twice causing a collision (once when loading to the server, and then again for the user).

Note that this is not an issue if it is in discojs, somehow by removing it from that scope the imported tfjs dependencies are less stable and causing something weird to happen in the engine that is keeping track of all this.

Note the key here is the tfjs engine which keeps track of variables in the background; this was discussed and worked on in #391.

Issue discojs side

If model.ts is in discojs, then the above causes no issue, however in order to load models from the file system we need the following code:

export async function model (): Promise<tf.LayersModel> {
  const file = tf.io.fileSystem('file:/./../../../mobilenetv2_builder/mobileNetV2_35_alpha_2_classes/model.json')
  return await tf.loadLayersModel(file)
}

However io.fileSystem is only available with tfjs-node, and if we use tfjs-node then discojs is not compatible with the browser.

Thus, we need to conditionally import tfjs-node, which I've tried unsuccessfully here using the ts doc for conditional model loading. The issue is that I need to import the type of tfjs-node for the io functionality we need, but we need to import it without importing the functionality (which should be doable, but I have not yet found how).

@martinjaggi
Copy link
Member

once we load via links only as in #494 , this might simplify our node/not structure even further? (even if it already works now with the current architecture)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working discojs Related to Disco.js server Related to the server
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants