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

Add Python SDK version string #2418

Merged
merged 4 commits into from
May 19, 2020
Merged

Add Python SDK version string #2418

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ada023e
add sdk version string
liuzhe-lz May 11, 2020
c684ff7
validation
liuzhe-lz May 11, 2020
846d47a
update
liuzhe-lz May 18, 2020
dfa68e8
replace sdk version string in make install
liuzhe-lz May 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ install-dependencies: $(NNI_NODE_TARBALL) $(NNI_YARN_TARBALL)
.PHONY: install-python-modules
install-python-modules:
#$(_INFO) Installing Python SDK $(_END)
sed -ie 's/$(NNI_VERSION_TEMPLATE)/$(NNI_VERSION_VALUE)/' src/sdk/pynni/nni/__init__.py
sed -ie 's/$(NNI_VERSION_TEMPLATE)/$(NNI_VERSION_VALUE)/' setup.py && $(PIP_INSTALL) $(PIP_MODE) .

.PHONY: dev-install-python-modules
Expand Down
1 change: 1 addition & 0 deletions deployment/pypi/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

@chicm-ms chicm-ms May 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

there a sed replacement statement in nni/Makefile for source/dev install

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@chicm-ms chicm-ms May 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

cd $(CWD)

Expand Down
2 changes: 2 additions & 0 deletions deployment/pypi/install.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ Copy-Item $CWD\..\..\src\nni_manager\package.json $CWD\nni
(Get-Content $CWD\nni\package.json).replace($NNI_VERSION_TEMPLATE, $NNI_VERSION_VALUE) | Set-Content $CWD\nni\package.json
cd $CWD\nni
yarn --prod
cd $CWD\..\..\src\sdk\pynni\nni
(Get-Content __init__.py).replace($NNI_VERSION_TEMPLATE, $NNI_VERSION_VALUE) | Set-Content __init__.py
cd $CWD
(Get-Content setup.py).replace($NNI_VERSION_TEMPLATE, $NNI_VERSION_VALUE) | Set-Content setup.py
python setup.py bdist_wheel -p $WHEEL_SPEC
2 changes: 2 additions & 0 deletions src/sdk/pynni/nni/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# Copyright (c) Microsoft Corporation.
# Licensed under the MIT license.

__version__ = '999.0.0-developing'
Copy link
Contributor

Choose a reason for hiding this comment

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

where this string is replaced with correct version? this string in setup.py is replaced in Makefile


from .env_vars import dispatcher_env_vars

if dispatcher_env_vars.SDK_PROCESS != 'dispatcher':
Expand Down