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

Fix make dev-install #2372

Merged
merged 2 commits into from
May 6, 2020
Merged

Fix make dev-install #2372

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f6cec72
fix dev install
liuzhe-lz Apr 24, 2020
a0f045b
Merge branch 'master' into fix-dev-install
liuzhe-lz Apr 26, 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
24 changes: 11 additions & 13 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,12 +173,12 @@ install-python-modules:
dev-install-python-modules:
#$(_INFO) Installing Python SDK $(_END)
mkdir -p build
ln -sf ../src/sdk/pynni/nni build
ln -sf ../src/sdk/pycli/nnicli build
ln -sf ../tools/nni_annotation build
ln -sf ../tools/nni_cmd build
ln -sf ../tools/nni_trial_tool build
ln -sf ../tools/nni_gpu_tool build
ln -sfT ../src/sdk/pynni/nni build/nni
ln -sfT ../src/sdk/pycli/nnicli build/nnicli
ln -sfT ../tools/nni_annotation build/nni_annotation
ln -sfT ../tools/nni_cmd build/nni_cmd
ln -sfT ../tools/nni_trial_tool build/nni_trial_tool
ln -sfT ../tools/nni_gpu_tool build/nni_gpu_tool
cp setup.py build/
cp README.md build/
sed -ie 's/$(NNI_VERSION_TEMPLATE)/$(NNI_VERSION_VALUE)/' build/setup.py
Expand All @@ -205,16 +205,14 @@ install-node-modules:
.PHONY: dev-install-node-modules
dev-install-node-modules:
#$(_INFO) Installing NNI Package $(_END)
rm -rf $(NNI_PKG_FOLDER)
Copy link
Member

Choose a reason for hiding this comment

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

It may need to test what if make install and then make 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.

You should never do that. Remove NNI first before switching to another installation setup.
We don't want to investigate what will happen if pip is requested to overwrite a normal package with an egg package.

Copy link
Member

Choose a reason for hiding this comment

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

Suggest to keep this line. Don't rely on human to do right things, and the normal install also have this line.

Copy link
Contributor Author

@liuzhe-lz liuzhe-lz Apr 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't agree. Removing this folder does NOT uninstall NNI from user's system, instead it leads to an ambiguous "partially installed and partially removed" state.
If a developer do something wrong, I prefer to fail early.

Copy link
Member

Choose a reason for hiding this comment

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

Any ambiguous state should be fixed in the Makefile. This change makes dev/normal installs script inconsistent, and dig an unnecessary pit. Anyway, if you still prefer, go ahead merge it.

Copy link
Contributor Author

@liuzhe-lz liuzhe-lz Apr 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

If you make install twice, it will be okay both with or without this folder, since pip and yarn are allowed to run multiple times with same options. Maybe this folder is removed in case the last bulid was corrupted.
But if someone make install then make dev-install, I don't know what will happen. Because the options feeded to pip are changed, and it's somewhat difficult to figure out pip's exact behaviour in such scenario.
If we want to ensure a clean install, we should trigger clean and uninstall targets instead of removing one or two folders. But as far as I know this is not the common practice of Makefile.
We can take a minute to discuss this issue on next scrum before merging it.

ln -sf ${PWD}/src/nni_manager/dist $(NNI_PKG_FOLDER)
ln -sfT ${PWD}/src/nni_manager/dist $(NNI_PKG_FOLDER)
cp src/nni_manager/package.json $(NNI_PKG_FOLDER)
sed -ie 's/$(NNI_VERSION_TEMPLATE)/$(NNI_VERSION_VALUE)/' $(NNI_PKG_FOLDER)/package.json
ln -sf ${PWD}/src/nni_manager/node_modules $(NNI_PKG_FOLDER)
ln -sf ${PWD}/src/webui/build -t $(NNI_PKG_FOLDER)
mv $(NNI_PKG_FOLDER)/build $(NNI_PKG_FOLDER)/static
ln -sfT ${PWD}/src/nni_manager/node_modules $(NNI_PKG_FOLDER)/node_modules
ln -sfT ${PWD}/src/webui/build $(NNI_PKG_FOLDER)/static
mkdir -p $(NASUI_PKG_FOLDER)
ln -sf ${PWD}/src/nasui/build $(NASUI_PKG_FOLDER)
ln -sf ${PWD}/src/nasui/server.js $(NASUI_PKG_FOLDER)
ln -sfT ${PWD}/src/nasui/build $(NASUI_PKG_FOLDER)/build
ln -sfT ${PWD}/src/nasui/server.js $(NASUI_PKG_FOLDER)/server.js

.PHONY: install-scripts
install-scripts:
Expand Down