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

[workflow] Add Phi-4 Finetuning Example #1308

Open
wants to merge 30 commits into
base: develop
Choose a base branch
from

Conversation

rajithkrishnegowda
Copy link
Collaborator

@rajithkrishnegowda rajithkrishnegowda commented Jan 24, 2025

This PR adds Phi-4 Finetuning example using Workflow API.

@rajithkrishnegowda rajithkrishnegowda changed the title WIP: Adding phi-4 model WIP: Add phi-4 model to OpenFL Jan 24, 2025
@rajithkrishnegowda rajithkrishnegowda marked this pull request as ready for review January 24, 2025 12:17
@rajithkrishnegowda rajithkrishnegowda changed the title WIP: Add phi-4 model to OpenFL Add phi-4 model to OpenFL Jan 24, 2025
Copy link
Collaborator

@MasterSkepticista MasterSkepticista left a comment

Choose a reason for hiding this comment

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

  • Why exactly do we need Gradio? How is it related to an FL tutorial?
  • The PR description/title is misleading. Please rename as "[workflow] Add Phi-4 Finetuning Example"
  • Consider asking the user to run fx experimental activate after installing OpenFL. This need not be part of the notebook execution cells.

@rajithkrishnegowda rajithkrishnegowda changed the title Add phi-4 model to OpenFL [workflow] Add Phi-4 Finetuning Example Jan 28, 2025
@rajithkrishnegowda
Copy link
Collaborator Author

  • Why exactly do we need Gradio? How is it related to an FL tutorial?
  • The PR description/title is misleading. Please rename as "[workflow] Add Phi-4 Finetuning Example"
  • Consider asking the user to run fx experimental activate after installing OpenFL. This need not be part of the notebook execution cells.
  1. Gradio is a user-friendly interface that simplifies the interaction with machine learning models. I have added a Federated Learning (FL) tutorial that allows users to input and submit the number of rounds for federated learning. This feature enhances interactivity and helps users better understand the iterative nature of federated learning.
  2. Changed the PR description/title
  3. added a note section for user to install openfl iv pip install -U openfl and to run fx experimental activate

@MasterSkepticista
Copy link
Collaborator

  1. I have added a Federated Learning (FL) tutorial that allows users to input and submit the number of rounds for federated learning. This feature enhances interactivity and helps users better understand the iterative nature of federated learning.
  • For (1), my recommendation is to remove gradio and the interactive element (if this existed for older notebooks, I suggest updating them too). A notebook interface is interactive enough. IMO adding GUI is out-of-place for a fine-tuning example. Furthermore, we really want to reduce the amount of version maintenance we do.
  • For (3), my suggestion was to actually not install openfl in the notebook. It is expected that the user has already installed OpenFL by the time they clone the repository. It is perhaps the first thing they are expected to do. In this (and all other) notebooks, a markdown cell can point the user to installation page on official docs and then the step to activate experimental APIs. Installing any packages specific to the notebook is fine, though.

@psfoley
Copy link
Contributor

psfoley commented Jan 28, 2025

  1. I have added a Federated Learning (FL) tutorial that allows users to input and submit the number of rounds for federated learning. This feature enhances interactivity and helps users better understand the iterative nature of federated learning.
  • For (1), my recommendation is to remove gradio and the interactive element (if this existed for older notebooks, I suggest updating them too). A notebook interface is interactive enough. IMO adding GUI is out-of-place for a fine-tuning example. Furthermore, we really want to reduce the amount of version maintenance we do.
  • For (3), my suggestion was to actually not install openfl in the notebook. It is expected that the user has already installed OpenFL by the time they clone the repository. It is perhaps the first thing they are expected to do. In this (and all other) notebooks, a markdown cell can point the user to installation page on official docs and then the step to activate experimental APIs. Installing any packages specific to the notebook is fine, though.
  1. Agree on this point. I'm personally a fan of gradio - and it would be useful to show in potentially other dedicated notebooks how other higher level interfaces can be built on top of the Workflow API, but in this notebook we want the focus to be LLM usage.
  2. OpenFL indeed should be installed ahead of time. workflow_interface_requirements.txt is only kept as separate for now because this is an experimental package. These will eventually be merged into the requirements of setup.py once the experimental label for the Workflow API is dropped. Installing packages needed for a particular notebook example is standard practice.

@rajithkrishnegowda
Copy link
Collaborator Author

  1. I have added a Federated Learning (FL) tutorial that allows users to input and submit the number of rounds for federated learning. This feature enhances interactivity and helps users better understand the iterative nature of federated learning.
  • For (1), my recommendation is to remove gradio and the interactive element (if this existed for older notebooks, I suggest updating them too). A notebook interface is interactive enough. IMO adding GUI is out-of-place for a fine-tuning example. Furthermore, we really want to reduce the amount of version maintenance we do.
  • For (3), my suggestion was to actually not install openfl in the notebook. It is expected that the user has already installed OpenFL by the time they clone the repository. It is perhaps the first thing they are expected to do. In this (and all other) notebooks, a markdown cell can point the user to installation page on official docs and then the step to activate experimental APIs. Installing any packages specific to the notebook is fine, though.

changes made in 6ebb95e

Copy link
Collaborator

@MasterSkepticista MasterSkepticista left a comment

Choose a reason for hiding this comment

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

Almost there. A few polish tips:

  1. Heading styles inconsistent across the notebook. Suggest using h1 for title (i.e. single # in markdown) and h2 for sections, h3 for sub-sections. Note that I don't mean to enforce using h1-h2-h3. Some notebooks are also good with just h1 and h3.
  2. Update PR description (since now not using gradio)
  3. Re-run the notebook and include cell outputs. As mentioned in previous notebook PRs, having cell outputs is useful for the user. This helps a user guess whether the notebook is worth their time investment very quickly.

@rahulga1
Copy link
Collaborator

Almost there. A few polish tips:

  1. Heading styles inconsistent across the notebook. Suggest using h1 for title (i.e. single # in markdown) and h2 for sections, h3 for sub-sections. Note that I don't mean to enforce using h1-h2-h3. Some notebooks are also good with just h1 and h3.
  2. Update PR description (since now not using gradio)
  3. Re-run the notebook and include cell outputs. As mentioned in previous notebook PRs, having cell outputs is useful for the user. This helps a user guess whether the notebook is worth their time investment very quickly.

@MasterSkepticista I have a different view on keeping the cell output in notebook. It hampers the readability, these examples are there for reference purpose to demonstrate capability.
Anyways we are in process of removing non-essential examples out of this repo and the one which we keep will be in included in CI, to ensure new changes are not breaking examples.

@rajithkrishnegowda
Copy link
Collaborator Author

Almost there. A few polish tips:

  1. Heading styles inconsistent across the notebook. Suggest using h1 for title (i.e. single # in markdown) and h2 for sections, h3 for sub-sections. Note that I don't mean to enforce using h1-h2-h3. Some notebooks are also good with just h1 and h3.
  2. Update PR description (since now not using gradio)
  3. Re-run the notebook and include cell outputs. As mentioned in previous notebook PRs, having cell outputs is useful for the user. This helps a user guess whether the notebook is worth their time investment very quickly.
  1. changes made in f785846
  2. updated PR description

Copy link
Collaborator

@kta-intel kta-intel left a comment

Choose a reason for hiding this comment

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

Looking good @rajithkrishnegowda, just some minor questions/comments

"metadata": {},
"outputs": [],
"source": [
"training_config = {\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I recommend cleaning up these parameters

  • remove parameters that are already set to their default values (for example, use_ipex, logging_strategy, etc.). It will improve readability and reduce cognitive overload.

  • it would be helpful to call out anything was specifically set to enable federated fine-tuning (i.e. setting 'num_train_epochs' to 1)

" \"warmup_ratio\": 0.2,\n",
"}\n",
"\n",
"peft_config = {\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, would be good to clean it up

"metadata": {},
"source": [
"After installation, activate experimental APIs using: \n",
"`fx experimental activate`"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question: since we are importing directly from openfl.experimental and are not using any fx commands, do we need to explicitly activate it?

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.

8 participants