Fix nerural network load model topology mismatch #84
Conversation
| @@ -251,7 +251,7 @@ class NeuralNetwork { | |||
| const modelJsonResult = await axios.get(filesOrPath.model, { | |||
| responseType: "text", | |||
There was a problem hiding this comment.
The response .data is already a string specifically because of the responseType: "text" here. If you don't specify a responseType then axios will default to parsing it as JSON.
| let modelMetadata = await axios.get(filesOrPath.metadata, { | ||
| responseType: "text", | ||
| }); | ||
| modelMetadata = JSON.stringify(modelMetadata.data); | ||
| modelMetadata = JSON.parse(modelMetadata); | ||
| modelMetadata = JSON.parse(modelMetadata.data); |
There was a problem hiding this comment.
If we want it as JSON then I think you could just remove the responseType: "text" and let axios parse it. Try this:
const metadataRes = await axios.get(filesOrPath.metadata);
this.meta = metadataRes.data;
There was a problem hiding this comment.
Thank you @lindapaiste! @ziyuan-linn take a look at this feedback and after you make any changes as you see fit I can merge!
| @@ -251,7 +251,7 @@ class NeuralNetwork { | |||
| const modelJsonResult = await axios.get(filesOrPath.model, { | |||
| responseType: "text", | |||
| }); | |||
| const modelJson = JSON.stringify(modelJsonResult.data); | |||
| const modelJson = modelJsonResult.data; | |||
| // TODO: browser File() API won't be available in node env | |||
| const modelJsonFile = new File([modelJson], "model.json", { | |||
There was a problem hiding this comment.
TBH this entire section of the code feels a bit odd and unnecessary. We are fetching the files, creating File objects, and then passing them to the TF model with tf.io.browserFiles. But TF is definitely capable of fetching files on its own so it seems like we shouldn't have to do that on our end. It seems like we should just be able to pass the URLs. That's a discussion for another time though, because maybe there's a reason that this is necessary and I'm just not seeing it.
There was a problem hiding this comment.
I believe this is necessary due to the way that the p5.js web editor stores the model files, but there may be a way to simplify it yes!
There was a problem hiding this comment.
Yeah that makes sense. I know the p5.js web editor well and the way that it handles uploaded files is complicated and messy because there is a URL that the user sees and uses in their code, "/model/model.json", and then there is the actual URL of the file which is some long string file name on an S3 server. So when TF is looking for other files with specific names in the same directory it's not gonna find them. We might be able to define a custom TF loader, but it's not as simple as I was thinking.
|
Merging! If there are further refinements we can make to simplify the file loading but retain compatibility with the web editor, we can do so in a new branches! |
This PR removes the
JSON.stringifycall onmodelJsonResult.dataandmodelMetadata.datasince Axios already returns those as strings.This addresses issue #78.