Include ATen/core/functional.h directly instead of torch/csrc/utils/functional.h.#16377
Include ATen/core/functional.h directly instead of torch/csrc/utils/functional.h.#16377ZolotukhinM wants to merge 3 commits intopytorch:masterfrom
Conversation
ZolotukhinM
requested review from
apaszke,
ebetica,
goldsborough,
pietern,
teng-li and
yf225
as code owners
facebook-github-bot
added
the
oncall: jit
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@ZolotukhinM has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
torch/lib/c10d/Utils.hpp
Outdated
There was a problem hiding this comment.
I think it could even be deleted now that it can use the c10 version directly. The copy was done originally to avoid including straight from csrc where all other includes were aten/c10. Feel free to ignore though, this is not critical.
There was a problem hiding this comment.
It doesn't look like functional.h is moved to c10 yet.
torch/csrc/jit/ir.h
Outdated
There was a problem hiding this comment.
This seems weird to me. The other user declarations make sense because they are reexporting "IR" stuff in the torch namespace. But then why would some random functional combinators also come along for the ride?
However, I am guessing you added this, because you would have had to renamespace a bunch of other stuff if you didn't? Maybe this suggests that we should have a global using namespace c10 in torch...
There was a problem hiding this comment.
Originally I put it here mostly to keep the changes mechanical (that's what we included with jit/functional.h anyway).
Now I removed using c10::filter from ir.h as it was used in only one place, but kept c10::fmap. Hopefully, it will keep annoying us so that we don't forget to cleanup this and unify namespaces.
ezyang
left a comment
ezyang
left a comment
There was a problem hiding this comment.
see comments, but I don't really want to bikeshed this too much
ZolotukhinM
force-pushed
the
inline-functional-h
branch
2 times, most recently
from
09d5c50 to
b3be5ff
Compare
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@ZolotukhinM is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@ZolotukhinM is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@ZolotukhinM is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
ZolotukhinM
force-pushed
the
inline-functional-h
branch
from
b3be5ff to
1f8ce38
Compare
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@ZolotukhinM is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@ZolotukhinM is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
ZolotukhinM
force-pushed
the
inline-functional-h
branch
from
1f8ce38 to
c8a222f
Compare
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@ZolotukhinM is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
ZolotukhinM
force-pushed
the
inline-functional-h
branch
from
c8a222f to
2b233a3
Compare
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@ZolotukhinM is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
4 participants
One more shim removed.