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

Add the platform interface and platform registry with fetch. #1708

Merged
merged 23 commits into from May 2, 2019

Conversation

Projects
None yet
4 participants
@nsthorat
Copy link
Collaborator

commented Apr 26, 2019

Create a platform interface and implement fetch.

Core will support Node and Browser as platforms. Environment flags are checked before setting the backend.

The same approach holds for dynamically importing node-fetch (using browser field for bundlers upstream).

This change is Reviewable

nsthorat added some commits Apr 26, 2019

@nsthorat nsthorat changed the title WIP: Add the platform interface and platform registry with fetch. Add the platform interface and platform registry with fetch. Apr 30, 2019

nsthorat added some commits May 1, 2019

@annxingyuan
Copy link
Collaborator

left a comment

Reviewable status: 0 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @nsthorat, @pyu10055, and @tafsiri)


src/platforms/node/platform_node.ts, line 28 at r1 (raw file):

export let systemFetch: (url: string, init?: RequestInit) => Promise<Response>;
export class PlatformNode {

Should this be extending the Platform class?

nsthorat added some commits May 1, 2019

@nsthorat
Copy link
Collaborator Author

left a comment

Reviewable status: 0 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @pyu10055, and @tafsiri)


src/platforms/node/platform_node.ts, line 28 at r1 (raw file):

Previously, annxingyuan (Ann Yuan) wrote…

Should this be extending the Platform class?

Nice catch, Done

@dsmilkov
Copy link
Member

left a comment

Reviewed 1 of 1 files at r2.
Reviewable status: 0 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @nsthorat, @pyu10055, and @tafsiri)


src/environment.ts, line 47 at r2 (raw file):

  setPlatform(platformName: string, platform: Platform) {
    this.platformName = platformName;

does it make sense to throw an error if two platforms are set in the same execution with different names? Also would be good to warn if two platforms are set with the same name (usually a sign of duplicated import in the bundle)


src/platforms/platform.ts, line 18 at r2 (raw file):

 */

export class Platform {

let's make this an interface. vscode (and other tools that use the tsc language services) can easily generate a class that implements Platform with all the necessary methods having a default implementation that throws "Not implemented yet". I want to avoid any possibility that platforms can carry a state in the base class.


src/platforms/platform.ts, line 19 at r2 (raw file):

export class Platform {
  fetch(path: string, requestInits?: RequestInit): Promise<Response> {

add a jsdoc what this fetch does


src/platforms/browser/platform_browser.ts, line 1 at r2 (raw file):

/**

since you have the platform name in the file name itself (platform_name.ts), how about having just a single platforms folder?


src/platforms/node/platform_node.ts, line 22 at r2 (raw file):

// We are wrapping this within an object so it can be stubbed by Jasmine.
export const getNodeFetch = {
  fetchImport: () => {

you can do this in a single line fetchImport: () => require('node-fetch');


src/platforms/node/platform_node_test.ts, line 31 at r2 (raw file):

    // Null out the system fetch so we force it to require node-fetch.
    // @ts-ignore
    platform_node.systemFetch = null;

it's unfortunate that you can't mock node-fetch. this is testing the internal details of PlatformNode.

@dsmilkov
Copy link
Member

left a comment

Reviewed 12 of 13 files at r1.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @nsthorat, @pyu10055, and @tafsiri)

@nsthorat
Copy link
Collaborator Author

left a comment

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @pyu10055, and @tafsiri)


src/environment.ts, line 47 at r2 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

does it make sense to throw an error if two platforms are set in the same execution with different names? Also would be good to warn if two platforms are set with the same name (usually a sign of duplicated import in the bundle)

Done


src/platforms/platform.ts, line 18 at r2 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

let's make this an interface. vscode (and other tools that use the tsc language services) can easily generate a class that implements Platform with all the necessary methods having a default implementation that throws "Not implemented yet". I want to avoid any possibility that platforms can carry a state in the base class.

+1, Done


src/platforms/platform.ts, line 19 at r2 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

add a jsdoc what this fetch does

Done


src/platforms/browser/platform_browser.ts, line 1 at r2 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

since you have the platform name in the file name itself (platform_name.ts), how about having just a single platforms folder?

Good point, we're not going to drop these, Done.


src/platforms/node/platform_node.ts, line 22 at r2 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

you can do this in a single line fetchImport: () => require('node-fetch');

Done


src/platforms/node/platform_node_test.ts, line 31 at r2 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

it's unfortunate that you can't mock node-fetch. this is testing the internal details of PlatformNode.

I know, but it's better than nothing since it can at least catch whether we did the require cache properly.

nsthorat added some commits May 2, 2019

@annxingyuan annxingyuan self-requested a review May 2, 2019

@annxingyuan
Copy link
Collaborator

left a comment

LGTM

@tafsiri
Copy link
Member

left a comment

Reviewed 8 of 8 files at r3.
Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @nsthorat, @pyu10055, and @tafsiri)


src/environment.ts, line 48 at r3 (raw file):

  setPlatform(platformName: string, platform: Platform) {
    if (this.platform != null) {
      console.warn(

maybe console.info? in general this should not be a problem right?


src/platforms/platform.ts, line 18 at r3 (raw file):

 */

export interface Platform {

Add jsdoc here outlining what a platform is/provides conceptually (externalizing some of the rational from the design doc).


src/platforms/platform.ts, line 18 at r3 (raw file):

 */

export interface Platform {

Why not have name be a property of the platform so that platformName doesn't need to be passed around as a separate variable in setPlatform?


src/platforms/platform.ts, line 20 at r3 (raw file):

GET

Small doc comment: remove GET, fetch can make other kinds of requests.


src/platforms/platform_node.ts, line 23 at r3 (raw file):

export const getNodeFetch = {
  // tslint:disable-next-line:no-require-imports
  fetchImport: () => require('node-fetch')

small naming question: why not importFetch?


src/platforms/platform_node.ts, line 29 at r3 (raw file):

export class PlatformNode implements Platform {
  fetch(path: string, requestInits?: RequestInit): Promise<Response> {
    if (systemFetch == null) {

how might someone override this? Could check for global.fetch and use that if present? For a use case see my response to tensorflow/tfjs#1514 where I suggest a user a diff fetch library to get extra functionality


src/platforms/platform_node_test.ts, line 27 at r3 (raw file):

    const platform = new PlatformNode();

    const savedFetch = platform_node.systemFetch;

won't this be null since fetch has not been called?


src/platforms/node/platform_node_test.ts, line 31 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

I know, but it's better than nothing since it can at least catch whether we did the require cache properly.

Quick question, is the goal to cache the result of require? I thought the getNodeFetch object was to enable the stub/spy in the test? Node will cache required modules automatically https://nodejs.org/api/modules.html#modules_caching

If getNodeFetch were a property on the instance would it be easier to override/inject dependencies? (while keeping the constructor free of params.

@nsthorat
Copy link
Collaborator Author

left a comment

Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @pyu10055, and @tafsiri)


src/environment.ts, line 48 at r3 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

maybe console.info? in general this should not be a problem right?

This is a problem since it's a function of order :)


src/platforms/platform.ts, line 18 at r3 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

Why not have name be a property of the platform so that platformName doesn't need to be passed around as a separate variable in setPlatform?

I don't want to encode the name in the platform because there could be a shared implementation of a platform (similar design pattern as backends). I've found it quite useful to have that decoupling in the backend world so want to keep the same pattern here.


src/platforms/platform.ts, line 18 at r3 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

Add jsdoc here outlining what a platform is/provides conceptually (externalizing some of the rational from the design doc).

Done, tried to keep it short because a platform should be defined as an implementation of this interface.


src/platforms/platform.ts, line 20 at r3 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…
GET

Small doc comment: remove GET, fetch can make other kinds of requests.

Done


src/platforms/platform_node.ts, line 23 at r3 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

small naming question: why not importFetch?

copied from existing code, but changed it


src/platforms/platform_node.ts, line 29 at r3 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

how might someone override this? Could check for global.fetch and use that if present? For a use case see my response to tensorflow/tfjs#1514 where I suggest a user a diff fetch library to get extra functionality

There is no more overriding of global.fetch explicitly. The user should pass their own fetch to model.load.

As a final escape hatch if that doesn't work, someone could overwrite the fetch of the platform though (e.g. ENV.platform.fetch = () => ...), or register a brand new platform.


src/platforms/platform_node_test.ts, line 27 at r3 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

won't this be null since fetch has not been called?

this is to make the test hermetic (systemFetch could be defined, we don't actually know by the time this test runs).


src/platforms/node/platform_node_test.ts, line 31 at r2 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

Quick question, is the goal to cache the result of require? I thought the getNodeFetch object was to enable the stub/spy in the test? Node will cache required modules automatically https://nodejs.org/api/modules.html#modules_caching

If getNodeFetch were a property on the instance would it be easier to override/inject dependencies? (while keeping the constructor free of params.

The goal is to unit test & cache, otherwise we would call require directly in the node implementation of fetch.

I don't want to rely on the node caching because I want to ensure that this test 1) busts the cache (by nulling out the import) and then 2) dynamically imports.

nsthorat added some commits May 2, 2019

@tafsiri

tafsiri approved these changes May 2, 2019

Copy link
Member

left a comment

Reviewed 2 of 3 files at r4.
Reviewable status: :shipit: complete! 3 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @nsthorat, and @pyu10055)


src/environment.ts, line 48 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

This is a problem since it's a function of order :)

Not sure I understand why its a problem. But nbd.


src/platforms/platform_node.ts, line 29 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

There is no more overriding of global.fetch explicitly. The user should pass their own fetch to model.load.

As a final escape hatch if that doesn't work, someone could overwrite the fetch of the platform though (e.g. ENV.platform.fetch = () => ...), or register a brand new platform.

I think we should still allow global fetch override. Models for example tend to hide the model.load call so user cannot call it directly. ENV.platform.fetch approach seems reasonable, but since we have been telling people to use the global I think staying compatible with that might be good. But up to you since ENV.platform.fetch is an option.

@nsthorat
Copy link
Collaborator Author

left a comment

Reviewable status: :shipit: complete! 3 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @pyu10055, and @tafsiri)


src/environment.ts, line 48 at r3 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

Not sure I understand why its a problem. But nbd.

because it's a setter, it's completely dependent upon who calls setPlatform first. usually if two things call setPlatform there's likely a bug with how you setup the build.


src/platforms/platform_node.ts, line 29 at r3 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

I think we should still allow global fetch override. Models for example tend to hide the model.load call so user cannot call it directly. ENV.platform.fetch approach seems reasonable, but since we have been telling people to use the global I think staying compatible with that might be good. But up to you since ENV.platform.fetch is an option.

Done. Only happens in Node.js now.

nsthorat added some commits May 2, 2019

@nsthorat nsthorat merged commit 5a4e300 into master May 2, 2019

2 checks passed

Trigger: ed916b23-8e01-4fda-85af-b7c8627d15a1 Summary
Details
cla/google All necessary CLAs are signed

@nsthorat nsthorat deleted the platform branch May 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.