Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Quickstart and remove autoCompletion in the doc #3861

Merged
merged 8 commits into from Jul 12, 2021

Conversation

kvartet
Copy link
Contributor

@kvartet kvartet commented Jun 22, 2021

some improvements:

  1. update the quickstart doc: change the v1 config to the v2 config and make it more user-friendly
  2. remove the nnictl auto-completion part in the doc, which is unsupported now
  3. fix typos in docs and codes: alogrithm, achine, complted
  4. fix the wrong header level

@@ -4,7 +4,7 @@ QuickStart
Installation
------------

We currently support Linux, macOS, and Windows. Ubuntu 16.04 or higher, macOS 10.14.1, and Windows 10.1809 are tested and supported. Simply run the following ``pip install`` in an environment that has ``python >= 3.6``.
Currently, NNI supports running on Linux, MacOS and Windows. Ubuntu 16.04 or higher, macOS 10.14.1, and Windows 10.1809 are tested and supported. Simply run the following ``pip install`` in an environment that has ``python >= 3.6``.
Copy link
Contributor

@liuzhe-lz liuzhe-lz Jun 25, 2021

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing it out, I will update it.

experimentName: MNIST # An optional name to distinguish the experiments
searchSpaceFile: search_space.json # The path to the search space file
trialCommand: python3 mnist.py # NOTE: change "python3" to "python" if you are using Windows
trialCodeDirectory: . # The path to the trial code
Copy link
Contributor

Choose a reason for hiding this comment

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

As a template, I think it's save to remove this line and ask the user to put config file in trial code's top directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, will update it


Run the **config_windows.yml** file from your command line to start an MNIST experiment.
Run the **config_windows.yml** file from your command line to start the experiment.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ooops, I removed config_windows.yml. I thought it's suggesting NNI behaves differently on Windows and Linux, which is not the case.
If it makes doc or quick start hard or complex, please add it back. Just copy config.yml and change python3 to python.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hahaha, a user asked in the WeChat group why there is no config_windows.yml example for v2.3, so I add it back?

Copy link
Contributor

Choose a reason for hiding this comment

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

Another choice is to tell the users to update the config. I don't know which is better.

Copy link
Contributor Author

@kvartet kvartet Jun 29, 2021

Choose a reason for hiding this comment

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

I modified all the config files in quickstart to config_detailed.yml, which including the inline search space and swapped the search space part and the trial code part so that users can config the experiment directly in the YAML file after writing the search space.
I didn't add config_windows.yml instead of asking users to update the trialCommand according to your suggestion.


**Step 1**: Write a ``Search Space`` file in JSON, including the ``name`` and the ``distribution`` (discrete-valued or continuous-valued) of all the hyperparameters you need to search.
Define a ``Search Space`` in the JSON file, including the ``name`` and the ``distribution`` (discrete-valued or continuous-valued) of all the hyperparameters you want to search.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can suggest embedded search space. It's more convenient in most cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

agree

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, so do we need to remove the JSON part? or suggest two ways to define the search space in the quickstart?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think in quick start we can remove JSON part.


**Step 1**: Write a ``Search Space`` file in JSON, including the ``name`` and the ``distribution`` (discrete-valued or continuous-valued) of all the hyperparameters you need to search.
Define a ``Search Space`` in the JSON file, including the ``name`` and the ``distribution`` (discrete-valued or continuous-valued) of all the hyperparameters you want to search.
Copy link
Contributor

Choose a reason for hiding this comment

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

agree

@@ -1,57 +0,0 @@
Auto Completion for nnictl Commands
===================================

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a future plan to support this feature? If yes, we can add a .. warning here that states the feature is broken in current releases and will be fixed in feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that @liuzhe-lz is the owner of this feature. - would you mind tell us the future plan?

Copy link
Contributor

@liuzhe-lz liuzhe-lz Jun 25, 2021

Choose a reason for hiding this comment

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

It is planned, with low priority.

Copy link
Contributor Author

@kvartet kvartet Jun 29, 2021

Choose a reason for hiding this comment

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

This feature has been deferred since v2.0, and we can't estimate when it will be implemented, so I temporarily removed it...

@QuanluZhang QuanluZhang merged commit fb3c596 into microsoft:master Jul 12, 2021
@khituras
Copy link
Contributor

The autoCompletion is still pointed to at https://nni.readthedocs.io/en/stable/Tutorial/QuickStart.html. It should be removed from there as well, I just searched for it until I found this issue.

@QuanluZhang
Copy link
Contributor

@khituras thanks for reporting this issue. would you mind to create a pr to help fix it?

@khituras
Copy link
Contributor

Thanks for the response @QuanluZhang.
I gladly would, however it has already been done per this issue. The current file at https://github.com/microsoft/nni/blob/master/docs/en_US/Tutorial/QuickStart.rst does ot mention the auto completion script.
However, the online documentation seems to be old. It needs to be updated from sources, I guess.

@QuanluZhang
Copy link
Contributor

Thanks for the response @QuanluZhang.
I gladly would, however it has already been done per this issue. The current file at https://github.com/microsoft/nni/blob/master/docs/en_US/Tutorial/QuickStart.rst does ot mention the auto completion script.
However, the online documentation seems to be old. It needs to be updated from sources, I guess.

oh, yes, sorry for the misleading. this has been fixed in the latest master. you can double check through this line https://nni.readthedocs.io/en/latest/ which is rendered from the latest master

@khituras
Copy link
Contributor

I see. I just followed the link from https://github.com/microsoft/nni. Perhaps this link should point to latest then?

@kvartet kvartet deleted the update-doc branch October 16, 2021 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants