Skip to content
This repository has been archived by the owner on Sep 18, 2024. It is now read-only.

[v2.0] Refactor code hierarchy (part 2) #2987

Merged
merged 13 commits into from
Oct 26, 2020
Merged

Conversation

liuzhe-lz
Copy link
Contributor

@liuzhe-lz liuzhe-lz commented Oct 19, 2020

Second part of code hierarchy refactor: Python package hierarchy and installation scripts. (last part: #2962 )

To install NNI from this branch:

    $ python setup.py develop

Moved Directories

Content Source Destination
HPO algorithms nni.XXX_{tuner,assessor,advisor} nni.algorithms.hpo.XXX_{tuner,assessor,advisor}
NAS algorithms nni.nas.{pytorch,tensorflow}.XXX nni.algorithms.nas.{pytorch,tensorflow}.XXX
Compression algorithms nni.compression.{torch,tensorflow}.{pruning,quantization} nni.algorithms.compression.{pytorch,tensorflow}.{pruning,quantization}
Feature engineering algorithms nni.feature_engineering.XXX_selector nni.algorithms.feature_engineering.XXX_selector
Internal SDK modules nni.{msg_dispatcher,protocol,platform,...} nni.runtime.{msg_dispatcher,protocol,platform,...}
Package utility nni.package_utils nni.tools.package_utils
Tool packages nni_{annotation,cmd,gpu_tool,trial_tool} nni.tools.{annotation,cmd,gpu_tool,trial_tool}
"nnicli" nnicli nni.experiment

Discussion

  1. This is a coarse grained refactor. There are packages I don't know where to place (e.g. nni.compression.torch.utils). I don't want to make everything perfect in PR, as long as they do not block future developing.
  2. Algorithms are moved to a separate folder. Because I think if it's hard to isolate algorithm packages, it will also be hard for users to implement custom algorithms.
  3. Public SDK APIs are not moved because I found it's hard to keep compatibility for codes like from nni.tuner import Tuner. So alternatively I moved all internal stuff to runtime.
  4. "NNI client" is renamed to "experiment", because it's mainly for experiment management and I'm looking forward to add non-client features like experiment creating (from dispatcher process).
  5. PyTorch is called torch in model compression and pytorch in NAS. I don't know which one is better. (Mao chose pytorch)

Summary

What part 2 includes:

  • Refactor Python code hierarchy
  • Port install scripts to python
  • Fix release build (building wheel)

What part 2 breaks:

  • import nni.xxx code
  • Unit test
  • Doc build

What part 2 not includes:

  • Add import shortcuts
  • Evaluate install on windows and mac
  • Update doc

@liuzhe-lz liuzhe-lz changed the base branch from v2.0 to master October 19, 2020 23:48
Copy link

@Tudor33 Tudor33 left a comment

Choose a reason for hiding this comment

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

Cool

@liuzhe-lz liuzhe-lz changed the base branch from master to v2.0 October 20, 2020 08:22
@ultmaster
Copy link
Contributor

ultmaster commented Oct 23, 2020

I think we should have an nni.common, and put all utility files from NAS, compression and HPO into it.

@liuzhe-lz
Copy link
Contributor Author

liuzhe-lz commented Oct 23, 2020

I think we should have an nni.common, and put all utility files from NAS, compression and HPO into it.

Can you provide more details about what functionality should be placed in nni.common?

@ultmaster
Copy link
Contributor

I think we should have an nni.common, and put all utility files from NAS, compression and HPO into it.

Can you provide more details about what functionality should be placed in nni.common?

For example, flops counter in compression, average meter in NAS, json2parameter, parameter2json in HPO.

cd docs/en_US/
sphinx-build -M html . _build -W
displayName: 'Sphinx Documentation Build check'
#- script: |
Copy link
Contributor

Choose a reason for hiding this comment

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

why annotate these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because some docs failed to build due to package path change. I don't want docs to block developing so they will be fixed later.

Copy link
Contributor

Choose a reason for hiding this comment

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

better to add a FIXME or TODO to mark these lines in case they are forgot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@liuzhe-lz
Copy link
Contributor Author

liuzhe-lz commented Oct 26, 2020

@SparkSnail
Quanlu suggests to rename nni_cmd to nni.tools.ctl or nni.tools.nnictl. What's your opinion?

@QuanluZhang
Copy link
Contributor

@liuzhe-lz for non-weight sharing nas algorithms, they are implemented the same way as hyper-parameter tuning algorithms. we should put them under "hpo" or "nas"?

@liuzhe-lz
Copy link
Contributor Author

liuzhe-lz commented Oct 26, 2020

@liuzhe-lz for non-weight sharing nas algorithms, they are implemented the same way as hyper-parameter tuning algorithms. we should put them under "hpo" or "nas"?

As long as users think they are NAS algorithms, I suggest to put them under nas.
If they are meant for NAS, we should refactor them to follow retiarii API sooner or later.
And I'm looking forward to adding "NNI package metadata" like xxx_algorithm/nni_info.py to let algorithms self-describe running mechanism.

@QuanluZhang
Copy link
Contributor

@liuzhe-lz for non-weight sharing nas algorithms, they are implemented the same way as hyper-parameter tuning algorithms. we should put them under "hpo" or "nas"?

As long as users think they are NAS algorithms, I suggest to put them under nas.
If they are meant for NAS, we should refactor them to follow retiarii API sooner or later.
And I'm looking forward to adding "NNI package metadata" like xxx_algorithm/nni_info.py to let algorithms self-describe running mechanism.

let's keep it for now, and finalize before v2.0 is released

@SparkSnail
Copy link
Contributor

SparkSnail commented Oct 26, 2020

@SparkSnail
Quanlu suggests to rename nni_cmd to nni.tools.ctl or nni.tools.nnictl. What's your opinion?

agree, nni.tools.ctl or nni.tools.nnictl is more straight forward for this module. nni.tools.cmd is more likely to means a common command line, nni.tools.ctl is specified for nnictl.

@SparkSnail SparkSnail closed this Oct 26, 2020
@SparkSnail SparkSnail reopened this Oct 26, 2020
@liuzhe-lz liuzhe-lz merged commit e21a698 into microsoft:v2.0 Oct 26, 2020
@liuzhe-lz liuzhe-lz deleted the v2-p2 branch October 27, 2020 18:56
@liuzhe-lz liuzhe-lz mentioned this pull request Nov 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants