Plugins revamp

[hhsecond]

Motivation and Context

Revamping plugin system to make at actually pluggable for different data types etc.

Description

• Introducing new io module. This will help users to use the import/export functionality of plugins through the program if they don't wan't to interact with the low-level hangar APIs
• We potentially can move other modules like dataset inside io module
• Test cases are work in progress
• Docs are work in progress

Types of changes

What types of changes does your code introduce? Put an x in all the boxes that apply:

• Documentation update
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Is this PR ready for review, or a work in progress?

• Ready for review
• Work in progress

How Has This Been Tested?

Put an x in the boxes that apply:

• Current tests cover modifications made
• New tests have been added to the test suite
• Modifications were made to existing tests to support these changes
• Tests may be needed, but they are not included when the PR was proposed
• I don't know. Help!

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have signed (or will sign when prompted) the tensorwork CLA.
• I have added tests to cover my changes.
• All new and existing tests passed.

[hhsecond]

@rlizzo There will be few more minute changes but it's in a good shape now to review especially because of the introduction of io module

[rlizzo]

Ok. I'd like to merge the changes in #130 before this, since they did a big overhaul of the test suit to simplify it's fixture setup (improving speeds in most cases)

[hhsecond]

Make sense. I'll finish my review soon

[rlizzo]

Thanks!

[rlizzo]

Hey @hhsecond lets connect about these changes on a call. I don't think I agree with moving to io directory to top level of src/hangar. Will get into the details over a vid chat.

[hhsecond]

Sure thing. I was reluctant too about it initially but then realized few reasons why should we do it. Let's talk

[codecov]

Codecov Report

Merging #134 into master will increase coverage by 0.15%.
The diff coverage is 78.74%.

@@            Coverage Diff             @@
##           master     #134      +/-   ##
==========================================
+ Coverage      95%   95.15%   +0.15%     
==========================================
  Files          63       61       -2     
  Lines       11203    11189      -14     
  Branches      998      968      -30     
==========================================
+ Hits        10643    10646       +3     
+ Misses        372      363       -9     
+ Partials      188      180       -8

Impacted Files	Coverage Δ
src/hangar/constants.py	100% <ø> (ø)	⬆️
tests/conftest.py	100% <ø> (ø)	⬆️
tests/test_cli.py	99.57% <100%> (-0.43%)	⬇️
tests/test_arrayset.py	100% <100%> (ø)	⬆️
src/hangar/external/__init__.py	100% <100%> (ø)
src/hangar/cli/utils.py	41.67% <41.67%> (ø)
src/hangar/external/base_plugin.py	62.5% <62.5%> (ø)
src/hangar/external/_external.py	68.42% <68.42%> (ø)
src/hangar/external/plugin_manager.py	75.61% <75.61%> (ø)
src/hangar/cli/cli.py	89.52% <94.05%> (+4.5%)	⬆️
... and 10 more

[rlizzo]

So i'm a bit baffled by the control flow here. There's some nice ideas (the cli changes seem to be really solid), but the manage_plugins.py file is a mess. Can you walk me through how exactaly plugins are discovered, loaded, and executed?

[elistevens]

What's the actual use case for plugins? I'm worried it's a lot of complexity and API surface area for benefit that isn't super clear from the outside.

[hhsecond]

@elistevens comment here #134 (comment) should give you more info. Lemme know what do you think

[elistevens]

Sounds great!

…

On Fri, Oct 18, 2019, 23:07 Sherin Thomas ***@***.***> wrote: @elistevens <https://github.com/elistevens> comment here #134 (comment) <#134 (comment)> should give you more info. Lemme know what do you think — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#134?email_source=notifications&email_token=AABBWICFKAAAKUGWM3EQM6LQPKPZVA5CNFSM4I7YWL2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBXGL6Y#issuecomment-544105979>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABBWICCQQI3VPQJX5TA3PDQPKPZVANCNFSM4I7YWL2A> .

[rlizzo]

Overall this is a really good design. There's a few things to clean up, but I really like this! Nice Job!

[rlizzo]

These and my previous single comments need to be changed. There are some serious errors and code smells in the cli methods, the tests need to be much more thorough, since they seem to have missed some really big issues.

[rlizzo]

This looks good to me. when the ci suite run's i'll merge!