No-scheme relative path should be routed to HTTP handler only in browser

[caisq]

• Currently, a call like tf.loadModel('./model.json') will be routed
  to tf.io.browserHTTPRequest regardless of whether the environment is
  browser or node.js. This will cause issues for node.js.
  This change makes it so that this routing of relative paths to
  browserHTTPRequest (using browser fetch) is done only in the
  browser. This implies that paths without a scheme is not supported.
  In other words, in node.js, a path given to tf.loadModel must have a
  scheme, e.g., tf.loadModel('file://./mode.json'),
  tf.loadModel('https://localhost/model.json').

Towards: tensorflow/tfjs#343

---

This change is 

[bileschi]

---

Review status: complete! 1 of 1 LGTMs obtained

---

Comments from Reviewable

[dsmilkov]

Review status: complete! 1 of 1 LGTMs obtained

---

src/models.ts, line 204 at r1 (raw file):

  if (typeof pathOrIOHandler === 'string') {
    const handlers = io.getLoadHandlers(pathOrIOHandler);
    if (handlers.length === 0 && ENV.get('IS_BROWSER')) {

does it make sense to have a "default" handler? Default handler gets called for relative paths. So if in the browser, register the httpHandler as the default handler. If node, register the fileHandler as the default handler. cc @nsthorat for thoughts.

---

src/models.ts, line 218 at r1 (raw file):

          `Cannot find any load handlers for path string '${pathOrIOHandler}'.`;
      if (ENV.get('IS_NODE')) {
        errorMsg +=

Can you add some unit tests that overwrite the IS_BROWSER ENV flag?

---

Comments from Reviewable

[nsthorat]

Review status: complete! 1 of 1 LGTMs obtained

---

src/models.ts, line 204 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

does it make sense to have a "default" handler? Default handler gets called for relative paths. So if in the browser, register the httpHandler as the default handler. If node, register the fileHandler as the default handler. cc @nsthorat for thoughts.

I think that might make sense, but would be good to weigh against the complexity of having two handlers registered for the same thing (introducing priority, etc).

Just a musing: I don't really see a 3rd place where a relative URL would be registered, so I don't have a strong negative reaction against the proposed change here.

---

Comments from Reviewable

[nsthorat]

Review status: complete! 1 of 1 LGTMs obtained

---

src/models.ts, line 204 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

I think that might make sense, but would be good to weigh against the complexity of having two handlers registered for the same thing (introducing priority, etc).

Just a musing: I don't really see a 3rd place where a relative URL would be registered, so I don't have a strong negative reaction against the proposed change here.

Sorry, I didn't see the description of the PR -- I don't think it's okay to force "file://" in node for relative paths.

---

Comments from Reviewable

[caisq]

Thank you all for the reviews so far. Let me know your thoughts, @nkreeger

Two options for node.js + tf.loadModel

1. Let relative paths without URL schemes be treated as paths on the file system
2. Always require URL schemes in node.js + tf.loadModel. I.e., throw an error when there is no URL scheme.

This PR implements option 2.

---

Review status: complete! 1 of 1 LGTMs obtained

---

Comments from Reviewable

[dsmilkov]

I'm definitely for #1 (Nikhil as well, based on his comment). Which is why I suggested an environment specific "default" handler that is responsible for handling relative paths.

---

Review status: complete! 1 of 1 LGTMs obtained

---

Comments from Reviewable

[nsthorat]

Yes -- I am also for #1.

---

Review status: complete! 1 of 1 LGTMs obtained

---

Comments from Reviewable

[bileschi]

This means that the same short program, executed in the two environments, will have different behavior. This is not big problem, but we should be very clear about this re. documentation etc.

---

Review status: complete! 1 of 1 LGTMs obtained

---

Comments from Reviewable

[nsthorat]

Yes, this is true, but this is how relative paths work in general in JavaScript, so it's not a divergence from expectations.

[dsmilkov]

Hi Shanqing,

Just ran across this PR by chance. Let me know if you need more info/input from me to push this forward. Thanks!

[caisq]

@dsmilkov Sorry - I backburnered this a little due to other things at hand. It's also because users of tfjs-node have at least one route of saving/loading working for them already, namely file://. But I do plan to get this done soon.

[dsmilkov]

Makes sense - I trust your priority judgement :)

[jafaircl]

Any update on this? [email protected] seems to have broken file:// loading

Edit: Nevermind. Just found the tf.io.fileSystem handler that doesn't appear to be documented anywhere.

[bileschi]

@jafaircl Can you point to where you looked for documentation? We will add the documentation for tf.io.fileSystem there.

[jafaircl]

@bileschi I looked on the js.tensorflow.org site here

[bileschi]

@tafsiri Does it make sense to add node-only API documentation here? It seems to me the answer is yes, but it would be nice to have some sort of tag to indicate when API calls won't be available in some environments.

@nkreeger thoughts?

See also @jafaircl 's response on stackoverflow here : https://stackoverflow.com/questions/53031887/unable-to-load-python-trained-model-in-tensorflow-js

[tafsiri]

@bileschi From what I can gather this would just be documenting the behavior of the function (tf.loadModel) in node vs the browser? I think that makes sense here.

In cases where the function would not be available in a particular environment i believe the intent was to put it in an environment namespace like tf.node or tf.browser but i'm not sure if that applies here.

[caisq]

Since this is not super high priority. I'll close the PR and re-open it later in the monorepo.