Skip to content

Move registry fully to c10#12077

Closed
Yangqing wants to merge 10 commits intopytorch:masterfrom
Yangqing:c10_registry
Closed

Move registry fully to c10#12077
Yangqing wants to merge 10 commits intopytorch:masterfrom
Yangqing:c10_registry

Conversation

@Yangqing
Copy link
Contributor

@Yangqing Yangqing commented Sep 26, 2018 •
edited
Loading

This does 6 things:

  • add c10/util/Registry.h as the unified registry util
    • cleaned up some APIs such as export condition
  • fully remove aten/core/registry.h
  • fully remove caffe2/core/registry.h
  • remove a bogus aten/registry.h
  • unifying all macros
  • set up registry testing in c10

Also, an important note that we used to mark the templated Registry class as EXPORT - this should not happen, because one should almost never export a template class. This PR fixes that.

@Yangqing Yangqing mentioned this pull request Sep 26, 2018
Closed
ezyang
ezyang reviewed Sep 26, 2018
View reviewed changes
c10/util/Registry.h
// TODO(jiayq): old versions of NVCC does not handle make_unique well
// so we are forced to use a unique_ptr constructor here. Check if it is
// fine to use make_unique in the future.
// return make_unique<DerivedType>(args...);

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

ezyang
ezyang reviewed Sep 26, 2018
View reviewed changes
c10/util/Registry.h
#define C10_DECLARE_TYPED_REGISTRY( \
RegistryName, SrcType, ObjectType, PtrType, ...) \
CAFFE2_API Registry<SrcType, PtrType<ObjectType>, __VA_ARGS__>* \
C10_IMPORT ::c10::Registry<SrcType, PtrType<ObjectType>, ##__VA_ARGS__>* \

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@ezyang
Copy link
Contributor

ezyang commented Sep 26, 2018

So... what's the directory structure plan for c10. Can we put all "public" headers at the top level? When does something go in "util"?

@Yangqing
Copy link
Contributor Author

Yangqing commented Sep 26, 2018

re directory structure, here is what I am thinking (for now):

  • for public header: we might want to have a c10/c10.h that dependent libraries use. Similar to aten/aten.h.
  • for internal code structure:
    • macros/ contains all macros and does not depend on anything else
    • util/ contains util functions, registry etc that is framework independent
    • (future) core/ contains core abstractions of framework, like tensors, devices etc. We can also choose to call it framework/
    • (future) serde/ contains serialization and deserialization, with PRIVATE HEADERS and PRIVATE DEPENDENCY on protobuf, effectively hiding our protobuf usage as a public header and that will resolve the protobuf version problem once and for all.

Does that make sense?

facebook-github-bot
facebook-github-bot reviewed Sep 26, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

Yangqing has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@Yangqing
Copy link
Contributor Author

Yangqing commented Sep 27, 2018

@pytorchbot retest this please - build seems to have timed out on windows.

@Yangqing Yangqing mentioned this pull request Sep 27, 2018
Closed
zdevito pushed a commit to zdevito/ATen that referenced this pull request Sep 27, 2018
@Yangqing @facebook-github-bot
Move registry fully to c10 (#12077)
6b86706 
Summary:
This does 6 things:

- add c10/util/Registry.h as the unified registry util
  - cleaned up some APIs such as export condition
- fully remove aten/core/registry.h
- fully remove caffe2/core/registry.h
- remove a bogus aten/registry.h
- unifying all macros
- set up registry testing in c10

Also, an important note that we used to mark the templated Registry class as EXPORT - this should not happen, because one should almost never export a template class. This PR fixes that.
Pull Request resolved: pytorch/pytorch#12077

Reviewed By: ezyang

Differential Revision: D10050771

Pulled By: Yangqing

fbshipit-source-id: 417b249b49fed6a67956e7c6b6d22374bcee24cf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ssnl ssnl ssnl left review comments

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@ezyang ezyang ezyang approved these changes

@apaszke apaszke Awaiting requested review from apaszke

@colesbury colesbury Awaiting requested review from colesbury

@gchanan gchanan Awaiting requested review from gchanan

@soumith soumith Awaiting requested review from soumith

@zdevito zdevito Awaiting requested review from zdevito

Assignees

No one assigned

Labels

open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Yangqing @ezyang @ssnl @facebook-github-bot