chore: Update install script for developers #91
Conversation
mitchnegus
changed the title
There was a problem hiding this comment.
Overall, I am very much in favor of moving away from this install script towards the stated vision. I think we should be careful with how that happens so that existing users do not have installations that break during this transition. We may want to consider a feature branch so that we can have reasonable commits to that branch without breaking existing installations?
The contributing/install documentation should also be adjusted for this PR. Assuming that the install script will continue to morph/disappear, we can either add a deprecation warning to the docs or use that feature branch to ensure consistency throughout this process.
| ####################################### | ||
| # Set up the default directory for output | ||
| # and ensure it has the correct permissions. | ||
| # Arguments: | ||
| # None | ||
| # Globals: | ||
| # DEFAULT_OUTPUT_DIR | ||
| # FIREWHEEL_GROUP | ||
| ####################################### | ||
| function setup_dirs() { | ||
| if ! mkdir -p "${DEFAULT_OUTPUT_DIR}"; then | ||
| err "FIREWHEEL failed to create default output directory: \"${DEFAULT_OUTPUT_DIR}\". Aborting." | ||
| exit 1 | ||
| fi | ||
|
|
||
| if ! chgrp "${FIREWHEEL_GROUP}" "${DEFAULT_OUTPUT_DIR}"; then | ||
| err "FIREWHEEL failed to alter group ownership of default output directory: \"${DEFAULT_OUTPUT_DIR}\". Aborting." | ||
| exit 1 | ||
| fi | ||
|
|
||
| if ! chmod -R g=u "${DEFAULT_OUTPUT_DIR}"; then | ||
| err "FIREWHEEL failed to permissions of default output directory: \"${DEFAULT_OUTPUT_DIR}\". Aborting." | ||
| exit 1 | ||
| fi | ||
| } |
There was a problem hiding this comment.
Does this functionality currently happen in firewheel init? If not, that may break some existing install processes that have assumptions about directory structure/ownership.
There was a problem hiding this comment.
I don't know if it does, but that seems like a reasonable place to add it if not.
| local mc_dir="${MC_DIR}_packages" | ||
| # Install the development version of FIREWHEEL | ||
| ${PYTHON_BIN} -m pip install ${PIP_ARGS} -e ${FIREWHEEL_ROOT_DIR}/[dev] | ||
| # Essential MCs (base, linux, vyos, etc.) were cloned; install them in development mode too | ||
| ${PYTHON_BIN} -m pip install ${PIP_ARGS} -e ${mc_dir}/firewheel_repo_base | ||
| ${PYTHON_BIN} -m pip install ${PIP_ARGS} -e ${mc_dir}/firewheel_repo_linux | ||
| ${PYTHON_BIN} -m pip install ${PIP_ARGS} -e ${mc_dir}/firewheel_repo_vyos |
There was a problem hiding this comment.
Just to be clear, this section seems like it assumes that FIREWHEEL and base, linux, vyos have been cloned locally in the path mc_dir and then installs them all in development mode. It seems prudent to check that this is true before installing.
There was a problem hiding this comment.
You are correct about this assumption. And sure, I can add that.
Since this was an intermediate step on the way towards eliminating this script altogether, I avoided trying to add too many checks. It shouldn't be difficult to add that check in, so I'll update.
mitchnegus
force-pushed
the
dev-no-mcs
branch
4 times, most recently
from
58e9587 to
2283b0d
Compare
This removes the `--no-clone` option in favor of requiring that repositories already be installed. Instead, a user will specify only whether or not to install in development mode, where in development mode repositories are installed as editable from the `MC_DIR` and otherwise they are installed directly using extras in the `pyproject.toml`
This captures a handful of the higher priority elements introduced by #91, while focusing on backwards compatibility. The primary modification here is to make more clear the distinction between cloned/uncloned editable/non-editable installs and have the docs adequately reflect the actions in the `install.sh` script. Previously, the documentation stated that the installer would clone our default model components, which was true before we transitioned to the package model. As we've made that transition, some of the implied functionality has been lost—right now, running the install script with no arguments will clone MCs but not install them... so still technically accurate, but somewhat misleading because we do not mention that any further actions to be taken. In a similar way, calling the script with `--no-clone` will also not clone MCs, but will also not install them even though one could argue that a familiar user (e.g., me) might expect `--no-clone` to not _clone_ the MC repos, but still install them using pip. There are a few other similar logic cases, but I won't discuss them at length. Instead, I'll summarize the behavior of the script as would be defined by this MR: | CLI Option | (clone) | `--no-clone` | |------------------|---------|---------------| | (production) | pip install model components from cloned versions | pip install model components from PyPI | | `--development` | pip install model components from cloned versions in editable mode | pip install model components from PyPI (same as above) | In all cases, when using this script, FIREWHEEL will be installed from this local version not PyPI. I don't think that's actually the case right now (looking at the line in the `install_firewheel` function), but this seems like it would be the expected behavior of running this script. Presumably, whoever's running the install script in the repo is doing it because they want to install that version of FIREWHEEL, not just have it turn around and download whatever the latest version is on PyPI.
| @@ -97,7 +97,7 @@ docs = [ | |||
| "sphinx-design", | |||
| ] | |||
| dev = [ | |||
| "firewheel[mcs,format,docs]", | |||
| "firewheel[format,docs]", | |||
There was a problem hiding this comment.
It looks like this change is needed a bit sooner as it seems like it has started to break the CI pipeline as there are some circular dependencies firewheel->firewheel-repo-base->firewheel. I'm not sure why it started breaking now, I can't get it to fail on my local systems with different python versions, but regardless, I'll likely move this into it's own PR (#127) to fix that. I'll wait to review the rest of this PR until it's no longer a draft.
There was a problem hiding this comment.
Ok, this really puzzled me, because I also couldn't reproduce it with a simple env. I dug a bit deeper and I think it's because in those CI jobs, it is a FIREWHEEL pipeline with build pinning setuptools 75–77 referencing a FIREWHEEL package (on PyPI) using a generic setuptools > 61.
The weirdness is highlighted by this pipeline which shows two seemingly contradictory items:
pip freeze --allshowssetuptools=76.1.0(this is correct)- the error message shows that doc8 has a deprecated License classifier (this is weird, because that error shouldn't be thrown until setuptools 78 🤔)
Tell the MCs to use a branch with matching build requirements and the problems seem to go away.
There was a problem hiding this comment.
This does beg the question if we should even have the [mcs] option at all. If you take them literally as dependency groups, they probably shouldn't... but we (and many) treat them as convenience "extras". (tox and others even name them like this; for posterity, I found a whole discussion here... with no good resolution.)
I like having the option (and would subscribe to the philosophy of the OP in that linked discussion), and understanding the mechanism suggests to me like this will only (?) be a problem when we change our build system requirements. I expect that will be relatively infrequent? (Though you are the arbiter of dependencies, so I'll leave that call to you 😅)
But I'm also a purist, so if we decide that this is a headache or otherwise abusive and just scrap the [mcs] extra altogether, I won't lose sleep.
This MR accomplishes a few things in service of eventually eliminating the
install.shscript. My end goal is that eventually everything in this script is either (1) performed by a call topip install ...or (2) absorbed into thefirewheel init.MC installation in Development Mode
The
[dev]optional dependencies extra is altered to no longer include installation of model components. Developers will presumably want to install MCs (base, linux, vyos) as editable packages. Users who wish to install MCs as standard (non-editable) packages within a development FIREWHEEL installation should use the combination of[mcs,dev]. See the setup process in the.gitlab-ci.ymlfile.No MC Repo Cloning
The script's ability to clone repos has been removed. Most non-development users of FIREWHEEL should be using
pip install firewheel[mcs]to install FIREWHEEL and its model components. Development users should be in control of which MC repos they install as editable, and so should be cloning, checking out branches, and installing those repos themselves.The
clone_reposfunctionality previously included in this file should be moved to the appropriate place on a system installer. However, since this version of the install script still performs thepip installof MCs in development mode, it needs to know where to find those repositories. Previously this was in the standardMC_DIRpath set via environment variable, but limitations of FIREWHEEL at the moment cause problems with duplicated MCs if editable packages are installed into an existing model component repository. To get around this, I've placed MC repository packages into a new directory created by appending_packagesto the existingMC_DIRvariable (e.g.,model_components_packages). This excludes those packages to be installed viapipfrom being included when callingfirewheel repository install $MC_DIR}. Eventually, we should find a workaround for this MC duplication (I think similar to #80, #81 but for editable installs) and we can put everything back inMC_DIR.Since the
clone_reposfunction has been removed, the--no-cloneoption has also been removed.Minor Adjustments
Some other things:
firewheel-2.6.0.tar.gzpackage in the event of an unsuccessful installationbuildpackage (install_firewheel_generic, which from what I could tell are no longer used)