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

fix: install dev package in its own prefix #209

Closed
wants to merge 2 commits into from

Conversation

P403n1x87
Copy link
Contributor

@P403n1x87 P403n1x87 commented May 24, 2023

Resolves: #211

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

@P403n1x87 P403n1x87 requested a review from a team as a code owner May 24, 2023 15:18
@P403n1x87 P403n1x87 requested review from juanjux and gnufede May 24, 2023 15:18
@P403n1x87 P403n1x87 force-pushed the fix/dev-package-prefix branch from a8b19fa to 27c9eed Compare May 24, 2023 15:23
avara1986
avara1986 previously approved these changes May 24, 2023
@mabdinur
Copy link
Contributor

mabdinur commented May 24, 2023

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

@mabdinur
Copy link
Contributor

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
Copy link
Contributor Author

P403n1x87 commented May 24, 2023

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.

riot/riot.py Show resolved Hide resolved
@P403n1x87 P403n1x87 force-pushed the fix/dev-package-prefix branch from 3c3a869 to 96df281 Compare May 25, 2023 08:59
@P403n1x87 P403n1x87 force-pushed the fix/dev-package-prefix branch 2 times, most recently from 0f7d0b8 to 67229cd Compare May 25, 2023 15:52
This prevents any dependency layers from removing the dev package
dependencies from the base environment.
@P403n1x87 P403n1x87 force-pushed the fix/dev-package-prefix branch from 67229cd to 59c014a Compare May 25, 2023 15:55
@mabdinur
Copy link
Contributor

mabdinur commented May 25, 2023

@P403n1x87
Copy link
Contributor Author

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.

@P403n1x87 P403n1x87 force-pushed the fix/dev-package-prefix branch from 7046f3d to e4073f6 Compare May 26, 2023 09:39
@mabdinur
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

mabdinur commented Jun 5, 2023

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
Copy link
Contributor Author

🤔 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
Copy link
Contributor Author

Superseded by #212

@P403n1x87 P403n1x87 closed this Jul 7, 2023
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

Successfully merging this pull request may close these issues.

Pinned requirements can conflict and override packages installed in the base virtual environment
3 participants