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.

Sign up for GitHub

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

Jump to bottom

Consider not checking in autogenerated core/{Tensor.h,TensorMethods.h} #24931

Open
zou3519 opened this issue Aug 20, 2019 · 7 comments
Open

Consider not checking in autogenerated core/{Tensor.h,TensorMethods.h} #24931

zou3519 opened this issue Aug 20, 2019 · 7 comments
Labels
module: build Build system issues module: cpp Related to C++ API triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Comments

@zou3519
Copy link
Contributor

zou3519 commented Aug 20, 2019

ATen/core/Tensor.h, ATen/core/TensorMethods.h are autogenerated and checked in. However, checking in autogenerated files is a recipe for merge conflicts; I've ran into 3 in the past two weeks. I think this is because git thinks it can merge two of these files from different branches together but they get merged together in a way that isn't consistent with how they get autogenerated.

Potential solutions:

  • stop checking in the autogenerated files
  • Have some sort of landing hook (I'm not sure if this is possible) where if the generated file doesn't match what is checked in, then we block the land.

cc @gchanan @ezyang
@gchanan gchanan added module: build Build system issues module: cpp Related to C++ API triage review labels Aug 20, 2019
@gchanan
Copy link
Contributor

gchanan commented Aug 20, 2019

This may be possible now because we don't actually need the Tensor class for the mobile build (we thought we did and we didn't build codegen for xplat). But I'm not sure if that's the long-term plan. Probably @ljk53 would know better.

@zou3519 zou3519 mentioned this issue Aug 21, 2019
Closed
@ezyang
Copy link
Contributor

ezyang commented Aug 21, 2019

Yes let's stop checking it in. We'll need to macro our way to victory but it seems far preferable.

@gchanan
Copy link
Contributor

gchanan commented Aug 21, 2019

I'm not sure why we need to macro our way to victory.

@ezyang
Copy link
Contributor

ezyang commented Aug 21, 2019 •
edited

My idea is that mobile would have a copy of Tensor.h but it just wouldn't have any methods on it. (But really any way to make this work would be fine by me.)

@ljk53
Copy link
Contributor

ljk53 commented Aug 22, 2019

@gchanan @ezyang - just FYI I've already updated xplat/caffe2/BUCK to build aten & torch with all the codegen stuff. If the original concern to check in Tensor.h was due to codegen then it's not the case any more :)

pearu added a commit to Quansight/pytorch that referenced this issue Aug 22, 2019
@pearu
Warn when autogenerated files mismatch, see issue pytorch#24931.
e792ade
@pearu
Copy link
Collaborator

pearu commented Aug 22, 2019

I was hit by the same issue in PR #24937 .
For debugging it, here's an implementation of showing the diff of nonmatching files:
https://github.com/pytorch/pytorch/pull/24937/files#diff-76aa8cb3d0fad99c5f761d08cbcb4d19R372

@zou3519 zou3519 mentioned this issue Aug 22, 2019
Closed
zou3519 added a commit that referenced this issue Aug 22, 2019
@zou3519
Don't generate named tensor methods to {Tensor,TensorMethods}.h
ac25a9b 
Don't generate named tensor methods to {Tensor,TensorMethods}.h when
BUILD_NAMEDTENSOR=0 in an attempt to prevent fewer merge conflicts.

We can either fix the root cause by solving
#24931, or, in the short term,
go through with this (hacky) solution.

Here's what happens:
1) We filter out all named tensor declarations if BUILD_NAMEDTENSOR=0 so
   that they never hit the codegen.
2) What is checked in as {Tensor,TensorMethods}.h comes from a
   BUILD_NAMEDTENSOR=0 build.
3) When BUILD_NAMEDTENSOR=1, then GEN_TO_SOURCE is automatically set to
   1. When working on a named tensor build, one must remember not to
   check in the generated code.

Test Plan:
- run ci [namedtensor ci]
zou3519 added a commit that referenced this issue Aug 22, 2019
@zou3519
Update on "Don't generate named tensor methods to {Tensor,TensorMetho…
610cace 
…ds}.h"

Don't generate named tensor methods to {Tensor,TensorMethods}.h

Don't generate named tensor methods to {Tensor,TensorMethods}.h when
BUILD_NAMEDTENSOR=0 in an attempt to prevent fewer merge conflicts.

We can either fix the root cause by solving
#24931, or, in the short term,
go through with this (hacky) solution.

Here's what happens:
1) We filter out all named tensor declarations if BUILD_NAMEDTENSOR=0 so
   that they never hit the codegen.
2) What is checked in as {Tensor,TensorMethods}.h comes from a
   BUILD_NAMEDTENSOR=0 build.
3) When BUILD_NAMEDTENSOR=1, then GEN_TO_SOURCE is automatically set to
   1. When working on a named tensor build, one must remember not to
   check in the generated code.

Test Plan:
- run ci [namedtensor ci]

gh-metadata: pytorch pytorch 25022 gh/zou3519/116/head
@gchanan
Copy link
Contributor

gchanan commented Aug 22, 2019

@ljk53 that's great! Let me try throwing up a patch that gets rid of the src checkin.

@pearu pearu mentioned this issue Aug 22, 2019
Closed
2 tasks
@ezyang ezyang added triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module and removed triage review labels Aug 26, 2019
facebook-github-bot pushed a commit that referenced this issue Sep 3, 2019
@pearu @facebook-github-bot
Implement indexing methods for sparse tensors (#24937)
f793a7c 
Summary:
Resolves #7416 .

This PR implements the following indexing methods for sparse tensors:
-  [x] `select`
-  [x] `index_select`

Note that this PR also modifies [gen.py](https://github.com/pytorch/pytorch/pull/24937/files#diff-76aa8cb3d0fad99c5f761d08cbcb4d19) that is not directly required to resolve the original issue but to work around a CI build issue reported in issue #24931 .
Pull Request resolved: #24937

Differential Revision: D17163796

Pulled By: ezyang

fbshipit-source-id: 06613301ec456d9ed3491b9ce48e804048600f09
zdevito pushed a commit to zdevito/ATen that referenced this issue Sep 3, 2019
@pearu @facebook-github-bot
Implement indexing methods for sparse tensors (#24937)
9fb6445 
Summary:
Resolves pytorch/pytorch#7416 .

This PR implements the following indexing methods for sparse tensors:
-  [x] `select`
-  [x] `index_select`

Note that this PR also modifies [gen.py](https://github.com/pytorch/pytorch/pull/24937/files#diff-76aa8cb3d0fad99c5f761d08cbcb4d19) that is not directly required to resolve the original issue but to work around a CI build issue reported in issue pytorch/pytorch#24931 .
Pull Request resolved: pytorch/pytorch#24937

Differential Revision: D17163796

Pulled By: ezyang

fbshipit-source-id: 06613301ec456d9ed3491b9ce48e804048600f09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
module: build Build system issues module: cpp Related to C++ API triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@ezyang @pearu @gchanan @zou3519 @ljk53