Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[Perl] - simplify aliasing strategy #15395

Merged
merged 1 commit into from
Jul 10, 2019
Merged

Conversation

tlby
Copy link
Contributor

@tlby tlby commented Jun 27, 2019

Description

There is some unnecessary complexity in the shorthand aliasing scheme this work aims to correct. We expect mx->nd->array(...) and AI::MXNet::NDArray->array(...) to be functionally equivalent and they are, but there are edge cases where the methods supported by the short and long names don't exactly align.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Responsibility of short name aliases moved from importer to imported module.
  • Retool constructor shorthand aliases to use AUTOLOAD rather than dynamic method creation to avoid namespace collisions between class and function names. For example, AI::MXNet::Context the class vs. AI::MXNet->Context() the shorthand for AI::MXNet::Context->new().
  • Folds the '_' suffixed shadow classes enhancements into the full name classes.
  • Retool of register() methods to not mangle stack traces.

Comments

  • I believe these changes are backwards compatible to the documented API, and brings the API as a whole closer to the designed intent.
  • Interesting edge cases to note here, I was unable to convert AI::MXNet::Gluon::Block->register() to use the AUTOLOAD scheme even though it seems to need the same class vs. method collision avoidance as in other places. Hopefully this can be dealt with in a future effort.

Copy link
Contributor

@sergeykolychev sergeykolychev left a comment

Choose a reason for hiding this comment

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

Cool changes, thank you!

 * alternate support for emulation of Python "import mxnet.someclass as sc"
   moving responsibility to the imported class instead of the importer.
 * simpler dispatch for constructor aliases to have fewer anonymous
   subroutines in stack traces on errors.
@tlby
Copy link
Contributor Author

tlby commented Jun 28, 2019

test_cuda_module.t didn't pass on the last run and I discovered I'd added a new ->register() call to AI/MXNet/CudaModule.pm, but didn't realize it contained multiple packages. it's now in the AI::MXNet::CudaModule package.

@anirudhacharya
Copy link
Member

@mxnet-label-bot add [pr-awaiting-merge]

@marcoabreu marcoabreu added the pr-awaiting-merge Review and CI is complete. Ready to Merge label Jun 28, 2019
@sergeykolychev sergeykolychev merged commit 6b2b927 into apache:master Jul 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-merge Review and CI is complete. Ready to Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants