-
Notifications
You must be signed in to change notification settings - Fork 2k
modularise clone op #2935
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
modularise clone op #2935
Conversation
dsmilkov
left a comment
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.
This all looks good to me. I agree with 2. It would be great to keep some chaining methods for dev ergonomics. Also wait for Nikhil. We can also sync on GVC if needed.
Reviewed 4 of 4 files at r1.
Reviewable status:complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @nsthorat, and @tafsiri)
tfjs-core/src/kernel_registry.ts, line 93 at r1 (raw file):
kernelName: string, backendName: string): KernelConfig { const key = makeKey(kernelName, backendName); if (kernelRegistry.has(key)) {
just an observation: this means that a backend could, technically, later overwrite clone with a backend-specific implementation. my first impression is that this is fine, but something to think about.
tfjs-core/src/ops/clone.ts, line 46 at r1 (raw file):
export const clone = op({clone_}); /**
either tie the jsdoc to a function/class/variable or use regular comment.
nsthorat
left a comment
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:
complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @nsthorat, and @tafsiri)
tfjs-core/src/kernel_registry.ts, line 93 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
just an observation: this means that a backend could, technically, later overwrite clone with a backend-specific implementation. my first impression is that this is fine, but something to think about.
I think that's the only point, right? If that wasn't the case, it shouldn't be a kernel at all.
tfjs-core/src/ops/clone.ts, line 39 at r1 (raw file):
*/ /** @doc {heading: 'Tensors', subheading: 'Creation'} */ function clone_<T extends Tensor>(x: T|TensorLike): T {
any idea how many ops are gonna be like this?
something to think about: IMO duplicating this code in each backend simplifies the mental model significantly, rather than having a default implementation and an override. Plus, where the code lives is now confusing. FromPixels already is confusing because of that.
tfjs-core/src/ops/clone.ts, line 55 at r1 (raw file):
const cloneKernelConfig: KernelConfig = { kernelName: Clone, backendName: BACKEND_AGNOSTIC, // this is a backend agnostic kernel
nit: remove this comment, the code is clear
tfjs-core/src/ops/clone.ts, line 62 at r1 (raw file):
}; const cloneGradientConfig: GradConfig = {
shouldn't this be in a separate file in gradients?
tafsiri
left a comment
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.
updated this PR based on GVC discussion.
Reviewable status:
complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, and @nsthorat)
tfjs-core/src/kernel_registry.ts, line 93 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
I think that's the only point, right? If that wasn't the case, it shouldn't be a kernel at all.
Removed backend agnostic kernel support
nsthorat
left a comment
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 1 of 4 files at r1, 5 of 5 files at r2.
Reviewable status:complete! 2 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, and @nsthorat)
dsmilkov
left a comment
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 3 of 5 files at r2.
Reviewable status:complete! 2 of 1 approvals obtained (waiting on @annxingyuan and @dsmilkov)
Strawman PR for discussion:
This PR modularises the clone op. But does so differently to how most ops will be modularised. There are three distinct things here:
clone(and thus do not list it in the kernel_names file). It is also done under the assumption that we always want clone available in tfjs/tfjs-core.To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
This change is