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.

Sign up for GitHub

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

Jump to bottom

flower-via-docker-compose example #2626

Merged
merged 52 commits into from Jan 25, 2024
Merged

flower-via-docker-compose example #2626

merged 52 commits into from Jan 25, 2024

Conversation

NikosVlachakis
Copy link
Contributor

@NikosVlachakis NikosVlachakis commented Nov 22, 2023 •
edited

Issue

Description

Related issues/PRs

Proposal

Explanation

Checklist

  • Implement proposed change
  • Write tests
  • Update documentation
  • Update changelog
  • Make CI checks pass
  • Ping maintainers on Slack (channel #contributions)

Any other comments?

@jafermarq jafermarq added the Examples Add or update a Flower example label Nov 22, 2023
jafermarq
jafermarq reviewed Nov 26, 2023
View reviewed changes
Copy link
Contributor

@jafermarq jafermarq left a comment

Choose a reason for hiding this comment

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

Hi @NikosVlachakis,

This is great :)

I left some comments below. In this first review i mostly focused on following the steps in the readme to run the code. The grafana dashboard is not populated with charts automatically. I believe the uid needs to be fixed both in the dashboard .json and the prometheus & grafana configs.

It would be ideal if could add a couple of links to some resources (maybe a grafana tutorial in their docs) so people running the example can look into the reference material in case they want to expand how the dashboard looks like or learn more about grafana. Having links to prometheus and cadvisor docs is also good.

I like how the docker compose file is generated.

examples/flower-via-docker-compose /README.md Outdated Show resolved Hide resolved
examples/flower-via-docker-compose /README.md Outdated Show resolved Hide resolved
examples/flower-via-docker-compose /config/provisioning/dashboards/default_dashboard.json Outdated Show resolved Hide resolved
examples/flower-via-docker-compose /config/provisioning/datasources/prometheus-datasource.yml Outdated Show resolved Hide resolved
examples/flower-via-docker-compose /helpers/generate_docker_compose.py Outdated Show resolved Hide resolved
examples/flower-via-docker-compose /requirements.txt Outdated Show resolved Hide resolved
@jafermarq jafermarq self-assigned this Nov 26, 2023
NikosVlachakis and others added 17 commits November 26, 2023 22:13
@NikosVlachakis
adding UID in grafana,prometheus configs and updating README.md file
8663dd0
@NikosVlachakis
adding flower + docker images
a02ed9b
@NikosVlachakis
changing grafana's UI default screen and adding system/application me…
e4c4546 
…trics
@jafermarq
mega dashboard
fd15bf8
@jafermarq
not stacking
9a6323a
@jafermarq
fixed time
e906b58
@NikosVlachakis
updating graphs grafana
6a8f88e
@NikosVlachakis
Merge branch 'main' of https://github.com/NikosVlachakis/flower
1a1b850
@NikosVlachakis
integrating flwr_datasets in data pipeline and updating README.md file
081e833
@jafermarq
simplifying strategy; load data once; bumped flwr 1.6; other minor ch…
1ac0b87 
…anges
@NikosVlachakis
updating generate_docker_compose.py by passing the data_percentage as…
f5620b5 
… an argument to the client.oy command
@NikosVlachakis
updating README.md + simplifying logic in load_data.py
e03d5aa
@jafermarq
argparse to compose generator; minor tweaks readme
af60339
@NikosVlachakis
adding random argument in generate_docker_compose file
711ea0b
@NikosVlachakis
small changes in readme.md file
5e4aaf8
@NikosVlachakis
remove unnecessary variables
c2ec018
@jafermarq
bump python3.10; --random flag
ee1a076
@jafermarq
Copy link
Contributor

@Robert-Steiner, please take a look at this example when you have some time. It looks completed to me eyes. But maybe you have some suggestions regarding how docker/docker-compose (or even grafana/prometheus) that we should incorporate to make this example more future proof.

jafermarq
jafermarq previously approved these changes Jan 23, 2024
View reviewed changes
Copy link
Contributor

@jafermarq jafermarq left a comment

Choose a reason for hiding this comment

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

@NikosVlachakis, this looks great!

@jafermarq jafermarq enabled auto-merge (squash) January 23, 2024 13:42
danieljanes
danieljanes reviewed Jan 24, 2024
View reviewed changes
Copy link
Member

@danieljanes danieljanes left a comment

Choose a reason for hiding this comment

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

Great PR, looking forward to merging this soon!

Just one small detail: for the images in public, could we remove them and link them similar to how we do it with the Flower logo?

@NikosVlachakis
Copy link
Contributor Author

Great PR, looking forward to merging this soon!

Just one small detail: for the images in public, could we remove them and link them similar to how we do it with the Flower logo?

Hey Daniel, thank you.
Do you mean to remove the entire public folder and use external (public) links for each image?

@danieljanes
Copy link
Member

Do you mean to remove the entire public folder and use external (public) links for each image?

Yes, exactly. When we link to images, we don't need to commit them to the repo, and it's clear where they come from for users.

auto-merge was automatically disabled January 25, 2024 11:03

Head branch was pushed to by a user without write access

@NikosVlachakis
Copy link
Contributor Author

Do you mean to remove the entire public folder and use external (public) links for each image?

Yes, exactly. When we link to images, we don't need to commit them to the repo, and it's clear where they come from for users.

Ok, I see, I removed the public folder and created publicly available urls for each image.

@danieljanes danieljanes enabled auto-merge (squash) January 25, 2024 16:04
@danieljanes danieljanes enabled auto-merge (squash) January 25, 2024 16:05
@danieljanes danieljanes merged commit 944afce into adap:main Jan 25, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danieljanes danieljanes danieljanes approved these changes

@jafermarq jafermarq jafermarq left review comments

@tanertopal tanertopal Awaiting requested review from tanertopal tanertopal is a code owner

Assignees

@jafermarq jafermarq

Labels
Examples Add or update a Flower example
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@NikosVlachakis @jafermarq @danieljanes