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

Small update to FLamby install instructions to avoid cluster failures. #287

Merged
merged 2 commits into from
Nov 18, 2024

Conversation

emersodb
Copy link
Collaborator

PR Type

Documentation

Short Description

Albumentations upgraded to 1.4.21 on October 31st. They now rely on simsimd, which is not compatible with our cluster. So we pin to 1.4.20 to make stuff work.

@@ -17,8 +17,10 @@ cd <fl4health_repository>
pip install --upgrade pip poetry
poetry install --with "dev, dev-local, test, codestyle"
cd <FLamby_repository>
pip install albumentations==1.4.20
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we add this to poetry's pyproject.toml instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question. This is specific to the FLamby environment. We don't use albumentations in our library but FLamby does in theirs. There is an issue using the most recent version of the library though. So this forces a pin while still allowing the rest of FLamby's dependencies to be installed

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. Is that the only dependency that is in flamby but not in fl4health?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No. Unfortunately there are a lot more in FLamby that are necessary to support the different datasets.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, I was just wondering if there was a requirements file somewhere that you could put this but looking into it here it seems like there isn't, so all good.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Your thought process is a good one. We actually used to have a requirements file for installing the requirements of FL4Health and FLamby together. The problem was that it ends up out of date fairly quickly, as it's not tied to our poetry or their repository. If you have better suggestions for maintaining the compatibility as a follow up PR, I'm open to it 🙂

Base automatically changed from dbe/move_to_future_annotations to main November 18, 2024 18:30
@emersodb emersodb merged commit a82a764 into main Nov 18, 2024
6 checks passed
@emersodb emersodb deleted the dbe/small_update_to_flamby_installs_for_cluster branch November 18, 2024 21:48
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.

2 participants