search

DataDog / riot

A Python virtual env builder and command runner
https://ddriot.readthedocs.io/
Apache License 2.0
23 stars 15 forks source link

fix: install dev package in its own prefix #209

Closed P403n1x87 closed 1 year ago

P403n1x87 commented 1 year ago

Resolves: https://github.com/DataDog/riot/issues/211

This prevents any dependency layers from removing the dev package dependencies from the base environment.

mabdinur commented 1 year ago

I tried installing riot from this branch and this caused the build_base_venvs job to fail (at least with python2.7)

mabdinur commented 1 year ago

Something interesting I noticed is that pip logs the following error in cherrypy when riot venvs are created (link)

ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
ddtrace 1.15.0.dev17+g5f8107360 requires typing-extensions, which is not installed

Is this expected? Would this fix also address this error?

P403n1x87 commented 1 year ago

Something interesting I noticed is that pip logs the following error in cherrypy when riot venvs are created (link)

ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
ddtrace 1.15.0.dev17+g5f8107360 requires typing-extensions, which is not installed

Is this expected? Would this fix also address this error?

It looks like the dev package is not being installed. That's when the dependencies are installed with this change. I wonder if there are some changes still needed to support the lockfiles, or if they have to be re-generated. Tagging @emmettbutler as I'm not that familiar with how that part of riot works.

mabdinur commented 1 year ago

With this fix we are able to build base envs in dd-trace-py ci: https://app.circleci.com/pipelines/github/DataDog/dd-trace-py/36413/workflows/5058b2b6-b4e1-45f1-8b34-531217eb88ac/jobs/2456864, however it causes a different set: https://app.circleci.com/pipelines/github/DataDog/dd-trace-py?branch=munir%2Funblock-ci

P403n1x87 commented 1 year ago

Ah! Having moved the library to a different prefix, we also need to update the PATH to allow it to find ddtrace-run 😢 . I'll look into that tomorrow.

mabdinur commented 1 year ago

Hmmm we still experiencing issues installing packages: import error, PR. Ideally these issues should be caught by riot tests. Can we add a regression test?

Also this hot fix has a lot complexity and could be difficult to debug/maintain. Can we simplify it a bit 😅

P403n1x87 commented 1 year ago

Hmmm we still experiencing issues installing packages: import error, PR. Ideally these issues should be caught by riot tests. Can we add a regression test?

Ah I think the slotscheck issue is to be expected since the dev package is no longer installed in the base venv. We would have to check whether the new site-packages is ending up in the PYTHONPATH. If not we need to somehow add it, but I'm not sure this is something to address in riot.

Also this hot fix has a lot complexity and could be difficult to debug/maintain. Can we simplify it a bit 😅

I'm afraid I don't have anything simpler to propose 🙁

mabdinur commented 1 year ago

I skipped the slotcheck

Hmmm we still experiencing issues installing packages: import error, PR. Ideally these issues should be caught by riot tests. Can we add a regression test?

Ah I think the slotscheck issue is to be expected since the dev package is no longer installed in the base venv. We would have to check whether the new site-packages is ending up in the PYTHONPATH. If not we need to somehow add it, but I'm not sure this is something to address in riot.

Also this hot fix has a lot complexity and could be difficult to debug/maintain. Can we simplify it a bit 😅

I'm afraid I don't have anything simpler to propose 🙁

I skipped the slotscheck in this PR and now most test runs are failing due import errors: https://app.circleci.com/pipelines/github/DataDog/dd-trace-py/36927/workflows/209ef960-c85f-4763-9ce1-727e5c1bdc92. I am not sure if this new issue should be fixed in riot or if we need to update the dd-trace-py tests? I don't think we can move forward with this change without addressing this issue.

@P403n1x87 any thoughts on what we should try next?

P403n1x87 commented 1 year ago

🤔 it looks like there are different kinds of failures. The ones failing to import envier seem to be missing the dev_pkg on the PYTHONPATH for some reason (and this is probably a bug in this change, but not obvious to me currently). The other failures (e.g. debugger) seem to be related to native extensions not being found. I wonder if we should do something more/different to ensure we can also find those native extensions.

P403n1x87 commented 1 year ago

Superseded by #212