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 missing package error when first using docker-compose setup #1158

Merged
merged 2 commits into from
Sep 6, 2022

Conversation

jjnesbitt
Copy link
Member

@jjnesbitt jjnesbitt commented Jul 5, 2022

Due to the following line

__version__ = get_distribution('dandiapi').version

The existing dev docker setup doesn't work as it's described in the README.md, as the dandiapi folder isn't copied into the container at the time of pip install, causing the dandiapi package to not be found. This wasn't an issue before, since we didn't rely on pip actually having knowledge of our dandiapi package to run django. However, due to our current versioning setup, any time our app is run, it runs the above line, to determine the current version. At that point, it finds that the dandiapi package isn't installed, causing an error.

We can't just copy the dandiapi directory into the container before pip install is run, since when we overwrite the current directory with our docker-compose setup, that folder will be gone, leaving us with the same problem. There's then two solutions:

1. Copy the dandiapi folder into the container at build time, before pip install. Then instead of mounting the entire directory with docker-compose, mount only the files/folders we need, so that the .egg-info folder isn't overwritten.
2. Add a step in the README to run pip install, which will then place a .egg-info folder in the cloned repo outside of the container. After that, it will always get mounted into the container when docker-compose is run, and behave normally.

This PR takes the latter approach, since imo maintaining an exhaustive list of all files needed in the container could lead to all kinds of issues, which would likely not be caught since the dev setup isn't tested.


My initial report was a red herring, this PR has been updated to address the real cause, outlined in #1158 (comment).

@jjnesbitt jjnesbitt requested a review from waxlamp July 5, 2022 17:25
@yarikoptic
Copy link
Member

@jwodder please have a look if aligns with your understanding of the moving pieces here

@jwodder
Copy link
Member

jwodder commented Jul 6, 2022

@AlmightyYakob Instead of using pkg_resources.get_distribution(), wouldn't you solve the problem by using importlib.metadata.version() (or importlib-metadata on pre-Python 3.8)? Aside from the fact that pkg_resources is deprecated, importlib.metadata operates on distribution packages rather than import packages, so it shouldn't need the code to be present at compile time.

@jjnesbitt
Copy link
Member Author

@AlmightyYakob Instead of using pkg_resources.get_distribution(), wouldn't you solve the problem by using importlib.metadata.version() (or importlib-metadata on pre-Python 3.8)? Aside from the fact that pkg_resources is deprecated, importlib.metadata operates on distribution packages rather than import packages, so it shouldn't need the code to be present at compile time.

Interesting, I wasn't aware of this. I can try that out, and if that solves our issues more elegantly, I'll close this PR in favor of that one.

@jjnesbitt
Copy link
Member Author

@AlmightyYakob Instead of using pkg_resources.get_distribution(), wouldn't you solve the problem by using importlib.metadata.version() (or importlib-metadata on pre-Python 3.8)? Aside from the fact that pkg_resources is deprecated, importlib.metadata operates on distribution packages rather than import packages, so it shouldn't need the code to be present at compile time.

What's your source for this behavior? In any test that I run, I need to install dandiapi prior, like with get_distribution. Otherwise, I'll get

importlib.metadata.PackageNotFoundError: dandiapi

@jwodder
Copy link
Member

jwodder commented Jul 25, 2022

@AlmightyYakob Yes, dandiapi needs to be installed no matter what, but the code shouldn't need to be there at install time. I interpreted this line from the OP:

as the dandiapi folder isn't copied into the container at the time of pip install, causing the dandiapi package to not be found

as referring to this folder and the dandiapi import package, not the distribution package. Is that not the case? My reading of django.Dockerfile indicates that the distribution package but not the import package is present when pip install runs in the Dockerfile, so the distribution package metadata should have access to the version number from version control. Could you clarify exactly what's happening with Docker and packaging and what the problem you're trying to solve is?

@jjnesbitt
Copy link
Member Author

My reading of django.Dockerfile indicates that the distribution package but not the import package is present when pip install runs in the Dockerfile, so the distribution package metadata should have access to the version number from version control. Could you clarify exactly what's happening with Docker and packaging and what the problem you're trying to solve is?

It seems I was a bit confused initially on this. You're correct in that when we run pip install in the container, the distribution package is available, and the version can be read in the final container (I've just checked that). The reason for the initial error is that since we do an editable install of dandiapi, when we mount the entire local directory with docker-compose, the local dandiapi.egg-info/ folder is overwritten (as is anything else in the directory). So at that point, there is no dandiapi distribution package installed, since the only record of that was kept in the dandiapi.egg-info folder.

I've tested just installing the package locally without --editable, and it seems to solve this issue, while still allowing for hot reloading. So I'll update this PR to reflect those changes. Your point about importlib is still valid though, so I'll probably incorporate that change as well.

@jjnesbitt jjnesbitt changed the title Update README to include extra setup step Fix missing package error when first using docker-compose setup Jul 25, 2022
@jjnesbitt jjnesbitt requested review from mvandenburgh and danlamanna and removed request for waxlamp July 25, 2022 21:29
Copy link
Member

@mvandenburgh mvandenburgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only potential problem I see here is anyone using docker to run things would lose the ability to easily make code changes to installed libraries (mainly thinking of our use of dandischema here), since we're doing non-editable mode pip install. I suppose we could just say, if you want the ability to do that you must run the python code natively, outside of docker. Either way, it's not something I've had to do often enough to be concerned about.

@jjnesbitt
Copy link
Member Author

The only potential problem I see here is anyone using docker to run things would lose the ability to easily make code changes to installed libraries (mainly thinking of our use of dandischema here), since we're doing non-editable mode pip install. I suppose we could just say, if you want the ability to do that you must run the python code natively, outside of docker. Either way, it's not something I've had to do often enough to be concerned about.

This was already the case though, wasn't it? Any third party libraries aren't present on the local machine itself, they're installed in the docker container's site packages. How does this change modify that setup?

@mvandenburgh
Copy link
Member

The only potential problem I see here is anyone using docker to run things would lose the ability to easily make code changes to installed libraries (mainly thinking of our use of dandischema here), since we're doing non-editable mode pip install. I suppose we could just say, if you want the ability to do that you must run the python code natively, outside of docker. Either way, it's not something I've had to do often enough to be concerned about.

This was already the case though, wasn't it? Any third party libraries aren't present on the local machine itself, they're installed in the docker container's site packages. How does this change modify that setup?

Looking back, I'm not sure what I was getting at here - I was referring to the case where you might do an editable install of your local dandischema repo, so that you could make changes to the code live, but this change doesn't prevent one from doing that. I think this change is fine 👍

@jjnesbitt jjnesbitt merged commit f977c4b into master Sep 6, 2022
@jjnesbitt jjnesbitt deleted the fix-local-dev branch September 6, 2022 18:29
@dandibot
Copy link
Member

dandibot commented Sep 8, 2022

🚀 PR was released in v0.2.46 🚀

@dandibot dandibot added the released This issue/pull request has been released. label Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants