Skip to content
This repository was archived by the owner on Aug 15, 2019. It is now read-only.

Add tf.util.fetch. #1648

Closed
wants to merge 13 commits into from
Closed

Add tf.util.fetch. #1648

wants to merge 13 commits into from

Conversation

annxingyuan
Copy link
Collaborator

@annxingyuan annxingyuan commented Mar 26, 2019

This PR fixes tensorflow/tfjs#1439

Changes

  • Added tf.util.fetch
  • Add node-fetch as dependency
  • Remove code that looks for global fetch in BrowserHTTPRequest

This change is Reviewable

@annxingyuan annxingyuan changed the title Add tf.util.fetch. WIP Add tf.util.fetch. Mar 26, 2019
@annxingyuan annxingyuan changed the title WIP Add tf.util.fetch. Add tf.util.fetch. Mar 26, 2019
@annxingyuan annxingyuan self-assigned this Mar 26, 2019
Copy link
Contributor

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 9 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, and @nsthorat)


package.json, line 56 at r1 (raw file):

    "test": "karma start",
    "run-browserstack": "karma start --browserstack",
    "test-benchmark": "cd integration_tests/benchmarks && yarn benchmark-travis && cd ../../",

do you mean to be deleting this here?


src/util.ts, line 671 at r1 (raw file):

/**
 * Returns a platform-specific implementation of `window.fetch`.

can you link to https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API


src/util.ts, line 682 at r1 (raw file):

 *  .then(response => {}) // handle response
 * ```
 */

@doc annotate this inside util? https://js.tensorflow.org/api/latest/#Util


src/util.ts, line 691 at r1 (raw file):

    if (ENV.get('IS_NODE')) {
      // tslint:disable-next-line:no-require-imports
      fetchFunc = require('node-fetch');

if we call tf.util.fetch multiple times, do we have to keep re-requiring this? can we cache this?


src/util.ts, line 693 at r1 (raw file):

      fetchFunc = require('node-fetch');
    } else {
      throw new Error(`Unable to find fetch polyfill.`)

semicolon

Copy link
Collaborator Author

@annxingyuan annxingyuan left a 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 @annxingyuan and @dsmilkov)


src/util.ts, line 671 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

can you link to https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API

Done


src/util.ts, line 682 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

@doc annotate this inside util? https://js.tensorflow.org/api/latest/#Util

Done


src/util.ts, line 691 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

if we call tf.util.fetch multiple times, do we have to keep re-requiring this? can we cache this?

Done


src/util.ts, line 693 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

semicolon

Done

@annxingyuan annxingyuan requested a review from nsthorat April 4, 2019 19:51
@annxingyuan annxingyuan closed this Apr 5, 2019
@annxingyuan
Copy link
Collaborator Author

Closed in favor of #1663

@annxingyuan annxingyuan deleted the add_fetch branch August 13, 2019 12:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add tf.util.fetch()
2 participants