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

chore: add algoliasearch dependency [BB-8083] #606

Conversation

0x29a
Copy link
Member

@0x29a 0x29a commented Nov 18, 2023

This is needed for open-craft/edx-enterprise#12 and open-craft/frontend-app-learner-portal-enterprise#2.

We can't add this dependency directly to edx-enterprise, because although it's designed to be able to run separately from edx-platform, currently it uses platform's dependencies.

@open-craft-grove
Copy link

Sandbox deploy request received. Deployment will start soon.

@open-craft-grove
Copy link

Sandbox deployment started.

@open-craft-grove
Copy link

Sandbox deployment failed.

Check failure logs here https://grove-stage-build-logs.nyc3.digitaloceanspaces.com/34602668-5566750713.log

Please check the settings and requirements and retry deployment by updating the pull request or posting a /grove sandbox update comment.

Copy link
Member

@Agrendalath Agrendalath left a comment

Choose a reason for hiding this comment

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

@0x29a,

although it's designed to be able to run separately from edx-platform

I didn't check this, but I thought edx-enterprise was an extension/plugin (like the edx-completion, etc.).

I don't understand why we need this change. When you install edx-enterprise (e.g., with pip install git+https://github.com/open-craft/edx-enterprise.git@0x29a/bb8083/per-user-algolia-key-nutmeg), the install_requires argument is verified. It should pull the dependencies from the package's requirements/base.in (you already added algoliasearch there in open-craft/edx-enterprise#12. The same thing happens when we install platform dependencies.

If the problem is that it tries to install 3.0.0, we could pin this dependency to "<3.0.0" in the base.in file (I know that the pins should be in the constraints files, but the setup.py of the edx-enterprise repo doesn't support the constraint files, so this is faster).

@0x29a
Copy link
Member Author

0x29a commented Nov 21, 2023

I didn't check this, but I thought edx-enterprise was an extension/plugin (like the edx-completion, etc.).

@Agrendalath, I guess what I wrote in the description is a poor re-phrasal of this comment:

############## OPEN EDX ENTERPRISE SERVICE CONFIGURATION ######################
# The Open edX Enterprise service is currently hosted via the LMS container/process.
# However, for all intents and purposes this service is treated as a standalone IDA.
# These configuration settings are specific to the Enterprise service and you should
# not find references to them within the edx-platform project.

Give this comment and that I didn't find base.txt in edx-enterprise, I concluded that all dependencies are hosted by edx-platform. It's a bit surprising to me that edx-enterprise is installing packages from non-compiled base.in. 🤔 (UPD: it looks like edx-completion does the same... I'm really curious why.)

Anyway, I uninstalled algoliasearch in the LMS container, then re-installed edx-enterprise and can confirm that it's installing algoliasearch==3.0.0. I'll pin it in edx-enteprise shortly, so we can close this PR. 👍

@0x29a
Copy link
Member Author

0x29a commented Nov 21, 2023

I'll pin it in edx-enteprise shortly, so we can close this PR.

Done.

@0x29a 0x29a closed this Nov 21, 2023
@0x29a 0x29a deleted the 0x29a/bb8083/per-user-algolia-key-nutmeg branch November 21, 2023 13:56
@open-craft-grove
Copy link

Sandbox update request received. Deployment will start soon.

@Agrendalath
Copy link
Member

@0x29a,

It's a bit surprising to me that edx-enterprise is installing packages from non-compiled base.in. 🤔 (UPD: it looks like edx-completion does the same... I'm really curious why.)

TL;DR: It's intentional because we use the same Python environment for the IDAs and all their dependencies. It would be too hard to manage dependencies without this. In some repos, we even omit compiled dependency lists while building Python packages to avoid using them by mistake.

Example

During the deployment, additional dependencies are installed sequentially, so every minor version change would reinstall tons of packages, which is not something we ever want to do. Let's say we bump the Django patch version in edx-platform to include a security fix. Then, we have the edx-enteprise package that also bumped the Django version, but doesn't have a new version tag yet. Installing it would downgrade the Django package, making our instance vulnerable. Now, we have tens/hundreds of these dependencies. Each of them would need to update its own dependencies and release a new version.

This process would make all previous version tags unusable, as they would downgrade other packages (I don't even want to imagine managing the compatibility with older named Open edX releases).

We could mitigate this by compiling dependencies for all packages (from the whole dependency graph, not only from the explicit requirements) in the deployment pipelines (right before their installation). However, with this many dependencies, it would be very slow and very error-prone. Perhaps other package installers have some native ways to handle such use cases, but migrating to them would be challenging (though the part after convincing everyone to use them could be rather straightforward).

@0x29a
Copy link
Member Author

0x29a commented Dec 8, 2023

@Agrendalath, ah, I think I understand now. Since *.in files don't specify exact versions, pip doesn't re-install a package if some specific version of it is already installed for edx-platform, and so we don't have to keep packages versions in sync across multiple dependencies. Thanks for explaining.

We could mitigate this by compiling dependencies for all packages (from the whole dependency graph, not only from the explicit requirements) in the deployment pipelines (right before their installation). However, with this many dependencies, it would be very slow and very error-prone. Perhaps other package installers have some native ways to handle such use cases, but migrating to them would be challenging (though the part after convincing everyone to use them could be rather straightforward).

Perhaps another option is what edx-enterprise already does, namely pulling platform's base.txt and treating it as "constraints" for pip-compile.

@Agrendalath
Copy link
Member

@0x29a,

Since *.in files don't specify exact versions, pip doesn't re-install a package if some specific version of it is already installed for edx-platform, and so we don't have to keep packages versions in sync across multiple dependencies. Thanks for explaining.

Yep, exactly. No problem!

Perhaps another option is what edx-enterprise already does, namely pulling platform's base.txt and treating it as "constraints" for pip-compile.

That's an interesting approach. However:

  1. This only works fine if you use the master branch of edx-platform. It doesn't guarantee the compatibility with previous releases. Pulling dependencies from each compatible named release would be more tricky. If we wanted to verify this against multiple releases, integration tests (like these) could be a better solution.
  2. I wonder if the benefits outweigh the potential extra maintenance - i.e., how many failures would be only false positives that block the CI until resolved in edx-platform.

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.

3 participants