Skip to content

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

Closed
0x29a wants to merge 1 commit into
opencraft-release/nutmeg.2from
0x29a/bb8083/per-user-algolia-key-nutmeg
Closed

chore: add algoliasearch dependency [BB-8083]#606
0x29a wants to merge 1 commit into
opencraft-release/nutmeg.2from
0x29a/bb8083/per-user-algolia-key-nutmeg

Conversation

@0x29a

@0x29a 0x29a commented Nov 18, 2023

Copy link
Copy Markdown

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
Copy Markdown

Sandbox deploy request received. Deployment will start soon.

@open-craft-grove

Copy link
Copy Markdown

Sandbox deployment started.

@open-craft-grove

Copy link
Copy Markdown

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.

@Agrendalath Agrendalath left a comment

Copy link
Copy Markdown
Member

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

0x29a commented Nov 21, 2023

Copy link
Copy Markdown
Author

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:
https://github.com/open-craft/edx-platform/blob/a5219364bd846a2d02f95dbe9f2550fc7211f9a2/lms/envs/common.py#L4579-L4583

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

0x29a commented Nov 21, 2023

Copy link
Copy Markdown
Author

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
Copy Markdown

Sandbox update request received. Deployment will start soon.

@Agrendalath

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

0x29a commented Dec 8, 2023

Copy link
Copy Markdown
Author

@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
Copy Markdown
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