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

Allow loading modules in Docker image #17228

Closed
wants to merge 2 commits into from
Closed

Conversation

erikjohnston
Copy link
Member

@erikjohnston erikjohnston commented May 22, 2024

This is done by mounting a /modules directory and installing packages into that.

One choice that I made was to ensure that package versions in /modules do not override those installed by Synapse. This ensures that people using custom modules cannot accidentally install an incompatible version of a package.

Closes #9944

This is done by mounting a `/modules` directory and installing packages
into that.
@erikjohnston erikjohnston marked this pull request as ready for review May 22, 2024 14:05
@erikjohnston erikjohnston requested a review from a team as a code owner May 22, 2024 14:05
Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

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

I'm happy with this approach, I think it's the best one we have.

I seem to remember someone in some meeting ages ago had something they didn't like about it, but honestly it seems like the best option we have.

Might be worth noting that this approach won't work (smoothly, at least) for libraries with native dependencies.

@erikjohnston
Copy link
Member Author

Might be worth noting that this approach won't work (smoothly, at least) for libraries with native dependencies.

Oh, hmm, true. I wonder if we should instead add a way of installing deps via the docker image (e.g. adding a new command to start.py like docker run synapse install <package>? Rather than installing on the host machine.

@MatMaul
Copy link
Contributor

MatMaul commented May 22, 2024

Current approach in this PR looks quite sane to me.

Might be worth noting that this approach won't work (smoothly, at least) for libraries with native dependencies.

Oh, hmm, true. I wonder if we should instead add a way of installing deps via the docker image (e.g. adding a new command to start.py like docker run synapse install <package>? Rather than installing on the host machine.

A lot of env uses a fresh container each time, which doesn't bode well with this proposal I believe.

@erikjohnston
Copy link
Member Author

I think we do need an approach for native packages here, we don't necessarily need to add support immediately but we should ensure we're not painting ourselves into a corner. For example, I think the current approach will sometimes work, sometimes not for native packages, which is a huge footgun.

Another thing we need to worry about is python versions, packages built for one version won't necessarily work after an upgrade.

A lot of env uses a fresh container each time, which doesn't bode well with this proposal I believe.

Yeah, we'd want to ensure that the modules are installed into a volume I think.

@erikjohnston
Copy link
Member Author

I'm not comfortable with this approach due to the aformentioned footguns of native packages / dependencies. I think there is a path forwards, but let's move this back to the issue.

@MatMaul
Copy link
Contributor

MatMaul commented May 22, 2024

Yeah, we'd want to ensure that the modules are installed into a volume I think.

Oh yes good point I didn't understand it that way, with both the volume mount and the entrypoint in the python script it seems rather solid to me. TBH with this PR you can already just do docker run -v modules:/modules pip install --target /modules mydep.

The native deps became less of a problem also I think : if it manages to compile within the docker environment then it should just run fine ?

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.

Identify, implement, and document a way to extend our Docker images
3 participants