Add an option to make frontend for not unintalling current environment's PL#2914
Add an option to make frontend for not unintalling current environment's PL#2914paul0403 wants to merge 3 commits into
make frontend for not unintalling current environment's PL#2914Conversation
|
Hello. You may have forgotten to update the changelog!
|
|
No changelog needed for build system |
| OQC_BUILD_DIR ?= $(MK_DIR)/frontend/catalyst/third_party/oqc/src/build | ||
| ENZYME_BUILD_DIR ?= $(MK_DIR)/mlir/Enzyme/build | ||
| COVERAGE_REPORT ?= term-missing | ||
| UNINSTALL_PL ?= ON |
There was a problem hiding this comment.
Thanks for adding this! Slightly clearer naming might be the following, because uninstall sounds like we might plain remove it?
| UNINSTALL_PL ?= ON | |
| FORCE_PL_INSTALL ?= ON |
| $(PYTHON) -m pip uninstall -y pennylane | ||
| $(PYTHON) -m pip install -e . --extra-index-url https://test.pypi.org/simple $(PIP_VERBOSE_FLAG) | ||
| else | ||
| $(PYTHON) -m pip install -e . --no-deps |
There was a problem hiding this comment.
@mudit2812 I tried locally, setup.py will still overwrite specific PL branch.
Say the current .dep-versions uses PL version dev10. So we have a couple cases:
- The developer's PL branch is less than dev10. In these cases,
setup.pywill still uninstall and bump it up to dev10 on testpypi. - The developer's PL is >= dev10, in which case it is kept, and
setup.pydoes nothing.
If we use --no-deps like I did here, we guarantee that the uninstall absolutely does not happen, but this does assume make frontend is only run after pip install -r requirements.txt. I do think this is a fair assumption, since this is the recommendation in the build guide.
However, I see the other side too: if a PL branch is too old, it should probably be forwarded by the dev.
What do you think?
There was a problem hiding this comment.
Yeah now that I think about it, the uninstall step merely forces picking up the exact PL version required by Catalyst when the installed package already has the same version tag (sometimes this happens, same tag, but different code). Disabling this does not affect the case where the installed PL version has a different tag, in which case pip will still look for the one provided by catalyst.
The only way around that that I know is --no-deps, we can do that but you can't assume just because requirement.txt was installed that we don't need the deps declared by the Catalyst package. In our setup, requirements.txt are build-time requirements, which are not the same as runtime requirements. Not installing the latter could lead to wonky configurations (e.g. a wrong lightning install, missing xdsl or malt package, etc.).
Of course if you know what you're doing you can proceed at your own risk, but you don't need a separate flag for that tbh, just run pip install -e . --no-deps.
There was a problem hiding this comment.
I had a discussion with @dime10 , and the two most favourable solutions seemed to be the following:
- Just add a note to the build from source guide saying that
make frontenddeletes the local installation of PennyLane and it must be reinstalled if needed. - Update the implementation of the new flag to look for a preinstalled version of pennylane (using
pip list) and then restoring it after installing Catalyst. This one is a bit more complicated, but may provide a better UX
| OQC_BUILD_DIR ?= $(MK_DIR)/frontend/catalyst/third_party/oqc/src/build | ||
| ENZYME_BUILD_DIR ?= $(MK_DIR)/mlir/Enzyme/build | ||
| COVERAGE_REPORT ?= term-missing | ||
| UNINSTALL_PL ?= ON |
There was a problem hiding this comment.
Could you add a description and update the help command?
6 participants
Context:
During development, it is often the case that people will need their local environment to be on a specific PennyLane branch, instead of a nightly package on testpypi. In these cases,
make frontenduninstalling the environment's current PL is very frustrating.Description of the Change:
Add an option to
make frontendto keep the local environment's current PL package.Benefits:
Better development experience.