Add Python SDK version string #2418
Conversation
| @@ -1,6 +1,8 @@ | |||
| # Copyright (c) Microsoft Corporation. | |||
| # Licensed under the MIT license. | |||
|
|
|||
| __version__ = '999.0.0-developing' | |||
There was a problem hiding this comment.
where this string is replaced with correct version? this string in setup.py is replaced in Makefile
| @@ -47,6 +47,7 @@ build: | |||
| cp $(CWD)../../src/nni_manager/package.json $(CWD)nni | |||
| sed -ie 's/$(NNI_VERSION_TEMPLATE)/$(NNI_VERSION_VALUE)/' $(CWD)nni/package.json | |||
| cd $(CWD)nni && $(NNI_YARN) --prod | |||
| sed -ie 's/$(NNI_VERSION_TEMPLATE)/$(NNI_VERSION_VALUE)/' $(CWD)../../src/sdk/pynni/nni/__init__.py | |||
| cd $(CWD) && sed -ie 's/$(NNI_VERSION_TEMPLATE)/$(NNI_VERSION_VALUE)/' setup.py && python3 setup.py bdist_wheel -p $(WHEEL_SPEC) | |||
There was a problem hiding this comment.
there a sed replacement statement in nni/Makefile for source/dev install
There was a problem hiding this comment.
When checking the Makefile I found a problem. I think dev-install should not replace the version string in setup.py.
Firstly developers are highly likely tracking their source files with git, and therefore the files should keep in sync with our repo. If a developer dev-install source code, do some editting, and then git commit -a, the replaced version string will be added to patch.
And I think the "developing" placeholder is a good version string for dev-install, because the target of symbol links may be any version. For example if I dev-install on v1.5 branch, and checkout v1.6 code tree later, the version string becomes misleading.
So I suggest only replace version string on normal install, and leave the placeholder unchanged for dev install.
There was a problem hiding this comment.
Yes, I agree that replacement on source generates annoying git changes.
The IT pipelines install nnimanager with source install and use depoyment install with docker based training service, and there is a version check in trial keeper...
https://github.com/microsoft/nni/blob/master/tools/nni_trial_tool/trial_keeper.py#L122
If we replace setup.py version and not replace nni.__version__ with source install, then in IT pipeline, the two types of version (nni.__version__ and pip distribution version) will be different, this may cause potential problem.
suggest to remove the existing replacement for setup.py in source install and update trial keeper version search pattern. https://github.com/microsoft/nni/blob/master/tools/nni_trial_tool/trial_keeper.py#L24
There was a problem hiding this comment.
I agree source install should keep in sync with pip install. But I think dev install does not necessarily need to be same as release version. I mean, 999.0-developing is a good version number for dev install I think, since it's always the "latest".
If dev install is currently used in release build, I feel okay to replace version string for now. But I believe it should use a separate target like fast-install.
So should we simply replace the version string anywhere for v1.6 release, or delay this PR for a refactor?
There was a problem hiding this comment.
build release pipeline is using deployment/pypi/Makefile pip install. Since 999.0-developing is a good version for dev, maybe remove the current sed replacement on setup.py for source/dev install ? This also can be done later.
Test PYPI: linux windows
I'm confused by the letter "v" problem.
The version string placeholder is "999.0.0-developing", without leading letter "v". But during installation, it's replaced by git tag, which is something like "v1.5".
If the leading letter "v" is desired, we should fix the placeholder; and if it should not be there, we should fix
NNI_VERSION_VALUEin makefiles. Personally I prefer not to include leading "v" inNNI_VERSION_VALUE, because that's Python's common practice as I know. If a module wants the leading "v", it can append it before placeholder and the replacement should work fine.