Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

We should avoid installing napps in dev mode #3

Open
italovalcy opened this issue Dec 23, 2021 · 4 comments
Open

We should avoid installing napps in dev mode #3

italovalcy opened this issue Dec 23, 2021 · 4 comments

Comments

@italovalcy
Copy link
Member

As discussed with @viniarck (and proposed by him), we should avoid installing Napps in dev mode as much as possible. This will avoid issues such as we saw in kytos-ng/flow_manager#61, as well as avoid unnecessary development dependencies. By doing so, we will end up having a more clean, secure, and simple docker build.

@viniarck
Copy link
Member

viniarck commented Jan 6, 2022

Team, here's a summary regarding -e (editable) mode installs and which ones should use it, based on how the core projects and the NApps have structured their setup.py:

  • kytos, python-openflow and kytos-utils all were meant to be pip installable installing their packages in the expected namespaces, so these ones when bundled together in the same Python runtime (in a Docker image for example) it's encouraged to pip install without dev mode to avoid any distribution issues.

  • NApps are designed to be loaded dynamically via kytos-utils via kytos under the napps.* namespace, and since their setup.py don't support bdist_wheel and the packages in the setup() call, so any attempt to try to bypass this leads to other sort of problems. With that being said, if pip installing NApps (as we have on our docker build), they should be installed with -e mode, the -e mode and their DevelopMode command won't install development dependencies, so it's OK, it's only going to install and enable the NApp in the NApps dir.

Initially I had the impression the -e mode for NApps were bringing the development dependencies but they aren't (the original issue was that we had certain install_requires bringing existing old dependencies re-installing previous versions with conflicting transitive dependencies like click, if a NApp installs again kytos with old version for instance), so no further actions are needed regarding these two previous points in our docker build, the only thing is to make sure we have consistent setup.py with minimal runtime dependencies when it executes the install_requires and egg_info, which the install_requires part has been addressed in the PRs that I tagged in the last hours (I'll keep fixing for the other NApps too)

@viniarck
Copy link
Member

viniarck commented Jan 6, 2022

Here's some ideas that I'd like to discuss and align just so we also have deterministic docker builds, bringing only run dependencies, and maintaining the setup.py as pip friendly as possible in terms of dependency resolution to avoid (overlapping/overwriting dependencies being installed by other NApps):

  1. install_requires should only install requirements/run.txt dependencies that the NApp requires
  2. NApps should always derive their requirements/*.txt from their requirements/*.in based on the dependency mgmt tool we use, currently pip-compile with pip-tools. That way, it facilitates maintenance, and whenever the project needs to bump dependencies we can rely on requirements/*.in and re-generate them again
  3. NApps shouldn't add kytos or its transitive dependencies on run.in / run.txt that are already shipped on kytos avoiding pip install resolution surprises and potential conflicts. kytos is only needed to be installed when developing locally with requirements/dev.txt, this goes back to the prior comment on this GitHub thread, NApps are designed to be dynamically loaded, so in the docker build, kytos will be already installed
  4. requirements/run.in should be as minimal as possible with major versions pinned to be deterministic
  5. If multiple NApps use the same library, for instance, requests, and they aren't shipped with kytos then it's recommended to use the same (major) version and pin it to avoid unexpected install deps resolution surprises during pip install on docker build, so if you have to introduce a new runtime library, make sure to inspect via pip freeze the docker image if the dependency had already been introduced and use the same version.
  6. Make sure install_requires clearly describes what the NApps uses to facilitate understanding, currently egg_info also bootstraps dependencies

And, in the future:

  1. Eventually, try to move away from install_requires in favor of PEP 517 with pyproject.toml
  2. Try to remove egg_info forced intalled dependencies, once we have the prior point 7) completed. I've prototyped a branch fix/no_egg_info on storehouse, and it worked, I haven't extensively explored it with docker build as well and bundling with other NApps, but I wouldn't expect surprises here as long as we have point 7 done or install_requires including whatever it needs to include. I've noticed though that some NApps on their tox.ini were forcing .egg-info removal see this on pathfinder for instance, so I'm not sure if this was a workaround for CI executions or something else, but it's an item to also cover it won't break if we move away from egg_info one day.
  3. Figure out a schedule to keep updating core and NApps dependencies, and document how to do it, just so we're ready to handle upgrades as needed maybe between major releases? The core projects are decoupled from the NApps, so upgrading NApps run.txt should be easier, kytos would be slightly more demanding to also update the requirements/dev.txt on NApps, this is also related to this issue of dependency management and major Python version that we mapped to discuss.

cc'ing @italovalcy @ajoaoff @jab1982 @rmotitsuki, let me know what you think, and if you have any suggestions or other improvements, I'll make sure to consolidate the points and map them as requirements in new issues

@viniarck
Copy link
Member

viniarck commented Jan 6, 2022

@italovalcy I've pushed a temporary branch with the the PRs mentioned here, just so we could try to build and push to the docker registry and try it out in the end-to-end tests, it's on this branch here https://github.com/viniarck/kytos-docker/tree/fix/build_install_requires_updated, I'll reach out to you to see the easiest way to have this image running in the end-to-end tests

@italovalcy
Copy link
Member Author

Here's some ideas that I'd like to discuss and align just so we also have deterministic docker builds, bringing only run dependencies, and maintaining the setup.py as pip friendly as possible in terms of dependency resolution to avoid (overlapping/overwriting dependencies being installed by other NApps):

  1. install_requires should only install requirements/run.txt dependencies that the NApp requires
  2. NApps should always derive their requirements/*.txt from their requirements/*.in based on the dependency mgmt tool we use, currently pip-compile with pip-tools. That way, it facilitates maintenance, and whenever the project needs to bump dependencies we can rely on requirements/*.in and re-generate them again
  3. NApps shouldn't add kytos or its transitive dependencies on run.in / run.txt that are already shipped on kytos avoiding pip install resolution surprises and potential conflicts. kytos is only needed to be installed when developing locally with requirements/dev.txt, this goes back to the prior comment on this GitHub thread, NApps are designed to be dynamically loaded, so in the docker build, kytos will be already installed
  4. requirements/run.in should be as minimal as possible with major versions pinned to be deterministic
  5. If multiple NApps use the same library, for instance, requests, and they aren't shipped with kytos then it's recommended to use the same (major) version and pin it to avoid unexpected install deps resolution surprises during pip install on docker build, so if you have to introduce a new runtime library, make sure to inspect via pip freeze the docker image if the dependency had already been introduced and use the same version.
  6. Make sure install_requires clearly describes what the NApps uses to facilitate understanding, currently egg_info also bootstraps dependencies

And, in the future:

  1. Eventually, try to move away from install_requires in favor of PEP 517 with pyproject.toml
  2. Try to remove egg_info forced intalled dependencies, once we have the prior point 7) completed. I've prototyped a branch fix/no_egg_info on storehouse, and it worked, I haven't extensively explored it with docker build as well and bundling with other NApps, but I wouldn't expect surprises here as long as we have point 7 done or install_requires including whatever it needs to include. I've noticed though that some NApps on their tox.ini were forcing .egg-info removal see this on pathfinder for instance, so I'm not sure if this was a workaround for CI executions or something else, but it's an item to also cover it won't break if we move away from egg_info one day.
  3. Figure out a schedule to keep updating core and NApps dependencies, and document how to do it, just so we're ready to handle upgrades as needed maybe between major releases? The core projects are decoupled from the NApps, so upgrading NApps run.txt should be easier, kytos would be slightly more demanding to also update the requirements/dev.txt on NApps, this is also related to this issue of dependency management and major Python version that we mapped to discuss.

cc'ing @italovalcy @ajoaoff @jab1982 @rmotitsuki, let me know what you think, and if you have any suggestions or other improvements, I'll make sure to consolidate the points and map them as requirements in new issues

@viniarck I'm glad you've brought all those points to the discussion. I agree with all of them and indeed we need to better manage the dependencies, installation mode, and the setup.py. Regarding item 9, I think that updating the Napps deps during the major release is fine. My only suggestion would be to document this in the best practices page

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants