-
Notifications
You must be signed in to change notification settings - Fork 949
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you link this change against other packages and make sure unit tests pass? If they fail, file an issue?
Reviewable status: 0 of 1 approvals obtained (waiting on @nsthorat and @pyu10055)
src/io/browser_http.ts, line 31 at r1 (raw file):
import {loadWeightsAsArrayBuffer} from './weights_loader'; const OCTET_STREAM_TYPE = 'application/octet-stream';
OCTET_STREAM_MIME_TYPE
src/io/browser_http.ts, line 32 at r1 (raw file):
const OCTET_STREAM_TYPE = 'application/octet-stream'; const JSON_TYPE = 'application/json';
JSON_MIME_TYPE
src/io/browser_http.ts, line 202 at r1 (raw file):
protected async loadJSONModel(): Promise<ModelArtifacts> { const modelConfigRequest = await this.getFetchFunc()( this.path as string, this.addAcceptHeader(JSON_TYPE));
do we need addAcceptHeader at all?
src/io/weights_loader.ts, line 47 at r1 (raw file):
} // Add accept header
dont need this anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 approvals obtained (waiting on @nsthorat)
src/io/browser_http.ts, line 202 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
do we need addAcceptHeader at all?
I think we should, for those servers that conform to the request headers, this would generate an error if the Accept type cannot be fulfilled, which will lead to a better error message.
src/io/weights_loader.ts, line 47 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
dont need this anymore
same reason as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 approvals obtained (waiting on @nsthorat and @pyu10055)
src/io/browser_http.ts, line 202 at r1 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
I think we should, for those servers that conform to the request headers, this would generate an error if the Accept type cannot be fulfilled, which will lead to a better error message.
But it's not actually true anymore, right? So we'd error in places we don't need to (since we actually accept whatever).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 approvals obtained (waiting on @nsthorat)
src/io/browser_http.ts, line 202 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
But it's not actually true anymore, right? So we'd error in places we don't need to (since we actually accept whatever).
This is suggestions for the server, and the server should send back 406 if it cannot fulfill. For the servers sent back wrong content types, they won't care about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 approvals obtained (waiting on @nsthorat and @pyu10055)
src/io/browser_http.ts, line 202 at r1 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
This is suggestions for the server, and the server should send back 406 if it cannot fulfill. For the servers sent back wrong content types, they won't care about this.
Okay -- have you tested this in either of the use cases Daniel mentioned in the original email? Would be good to make sure that's true so we don't have to do another patch release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 approvals obtained (waiting on @nsthorat)
src/io/browser_http.ts, line 202 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
Okay -- have you tested this in either of the use cases Daniel mentioned in the original email? Would be good to make sure that's true so we don't have to do another patch release.
I believe it should work based on what he has described. Because the header has been set before, and the server still returns the data but with wrong content type. I will confirm with him.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 approvals obtained (waiting on @nsthorat and @pyu10055)
src/io/browser_http.ts, line 202 at r1 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
I believe it should work based on what he has described. Because the header has been set before, and the server still returns the data but with wrong content type. I will confirm with him.
How about instead just use yalc to publish this package locally and test it against a model we've hosted on GCP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 approvals obtained (waiting on @nsthorat and @pyu10055)
src/io/browser_http.ts, line 202 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
How about instead just use yalc to publish this package locally and test it against a model we've hosted on GCP?
Okay following up, I still think we should remove this. We accept anything, so this is just 1) extra code complexity and 2) opportunity to break. Let's just remove the accept header entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r2.
Reviewable status: complete! 1 of 1 approvals obtained
fix tensorflow/tfjs#1188
Removed the logic to check response content-type header for model and weight files, this allows loading models from servers that do not produce correct content-type header.
This change is