Refactor pip install by aoyulong · Pull Request #1109 · flagos-ai/FlagScale

aoyulong · 2026-02-06T01:57:17Z

PR Category

Tools

PR Types

Improvements

PR Description

Refactor the installation process for FlagScale to make it easier for users to install with pip.
FLAGSCALE_PLATFORM=cuda FLAGSCALE_TASK=train pip install --no-build-isolation . -vvv

xmhubj · 2026-02-06T02:49:08Z

pyproject.toml

-dynamic = ["dependencies"]

 [project.urls]
-Homepage = "https://github.com/flagos-ai/FlagScale"


Not sure why we changed the url to FlagOpen.

@xmhubj No. We should use flagos-ai.

xmhubj · 2026-02-06T03:20:17Z

setup.py

-    return result

+# Version is defined in pyproject.toml - keep in sync
+FLAGSCALE_VERSION = "1.0.0"


It's better to get the version from pyproject.toml

xmhubj · 2026-02-06T03:20:48Z

setup.py


 setup(
-    name="flag_scale",
+    name="flagscale",


Copilot

Pull request overview

This PR refactors the FlagScale installation process to streamline pip-based installations and modernize the CLI framework. The changes enable users to install FlagScale with FLAGSCALE_PLATFORM=cuda FLAGSCALE_TASK=train pip install --no-build-isolation ., providing a more standard Python packaging experience.

Changes:

Renamed package from "flag_scale" to "flagscale" with simplified package structure
Migrated CLI framework from click to typer for better type safety and modern Python practices
Introduced --only-pip installation mode to skip apt packages and source builds
Refactored setup.py to conditionally run install.sh based on build isolation detection
Consolidated version management (now defined in pyproject.toml, setup.py, and flagscale/init.py)

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
version.py	Removed - version now managed in pyproject.toml and flagscale/init.py
init.py	Removed - no longer needed at repository root
.coveragerc	Removed configuration file
pyproject.toml	Updated package name to "flagscale", removed nonexistent pip version constraint, simplified configuration
setup.py	Complete rewrite to support conditional installation based on build isolation, now only includes minimal dependencies
flagscale/init.py	New file managing version with fallback mechanism using importlib.metadata
flagscale/cli.py	Complete rewrite using typer instead of click, added new `run` command and improved command structure
flagscale/run.py	New file extracting core logic from root run.py for modular imports
run.py	Refactored as backward compatibility wrapper importing from flagscale.run
requirements/common.txt	Added typer>=0.9.0 for CLI framework
tools/install/install.sh	Added --only-pip flag with documentation and examples
tools/install/utils/pkg_utils.sh	Implemented is_only_pip() and updated should_install_src() to respect --only-pip mode
tools/install/install_dev.sh	Added only-pip mode check in install_src()
tools/install/cuda/install_base.sh	Added only-pip mode check to skip apt packages
tools/install/cuda/install_train.sh	Added only-pip mode check in install_src()
tools/install/cuda/install_serve.sh	Added only-pip mode check in install_src()
flagscale/train/megatron/training/training.py	Added eval_iters parameter to evaluate() function signature

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-06T03:31:06Z

setup.py

+    result = subprocess.run(cmd, check=False)
+    if result.returncode != 0:
+        raise RuntimeError(f"Installation failed with exit code {result.returncode}")


The subprocess.run() call uses check=False and then manually raises an exception if returncode != 0. This bypasses Python's standard error handling. Either use check=True to let CalledProcessError be raised automatically, or if you need custom error handling, consider capturing stdout/stderr for better error messages.

Suggested change

result = subprocess.run(cmd, check=False)

if result.returncode != 0:

raise RuntimeError(f"Installation failed with exit code {result.returncode}")

try:

subprocess.run(cmd, check=True)

except subprocess.CalledProcessError as exc:

raise RuntimeError(f"Installation failed with exit code {exc.returncode}") from exc

Copilot · 2026-02-06T03:31:06Z

flagscale/cli.py

+    args = [
+        "run.py",
+        f"--config-path={config_path}",
+        f"--config-name={config_name}",
+        f"action={action}",
+    ]
+    if extra_args:
+        args.extend(extra_args)
+    sys.argv = args


The run_task function sets sys.argv[0] to "run.py" which doesn't match the actual command being invoked ("flagscale"). This could cause issues with error messages or help text that reference sys.argv[0]. Consider using a more accurate argv[0] value like "flagscale" or the actual task name.

Copilot · 2026-02-06T03:31:06Z

flagscale/cli.py

+    args = [
+        "flagscale",
+        f"--config-path={config_path}",
+        f"--config-name={config_name}",
+        f"action={action.value}",
+    ]
+    if overrides:
+        args.extend(overrides)
+
+    typer.echo(f"Running: {' '.join(args)}")
    sys.argv = args
    run_main()


The "run" command uses sys.argv[0] = "flagscale" at line 116, which is inconsistent with the run_task helper function that uses "run.py". This inconsistency could lead to confusion. Standardize on one approach throughout the CLI implementation.

Copilot · 2026-02-06T03:31:07Z

flagscale/cli.py

+    stop: bool = typer.Option(False, "--stop", help="Stop serving"),
+    test: bool = typer.Option(False, "--test", help="Test serving"),
+    tune: bool = typer.Option(False, "--tune", help="Auto-tune"),
+    port: int | None = typer.Option(None, "--port", "-p", help="Server port"),


The parameter name "port" conflicts with the typer.Option() name. In the serve() function, the parameter is named "port" but the option flag is also "-p" (short for --port), which could be confused with --config's "-c" flag. Consider using a different short flag like "-P" for port to avoid confusion, or remove the short flag entirely since --port is self-explanatory.

Suggested change

port: int | None = typer.Option(None, "--port", "-p", help="Server port"),

port: int | None = typer.Option(None, "--port", help="Server port"),

Copilot · 2026-02-06T03:31:07Z

setup.py

+    packages=["flagscale"],
+    package_dir={"flagscale": "flagscale"},


The setup function only declares a single package "flagscale", but this may not include all subpackages. Consider using setuptools.find_packages() or explicitly listing all subpackages (like flagscale.train, flagscale.serve, etc.) to ensure all modules are properly packaged. The current configuration relies on pyproject.toml's package-finding, but setup.py should be self-contained for backward compatibility.

Copilot · 2026-02-06T03:31:07Z

flagscale/train/megatron/training/training.py

    verbose=False,
    non_loss_data_func=None,
    extra_valid_index=None,
+    eval_iters=None,


The eval_iters parameter is added to the function signature but never used in the function body. This creates dead code and potential confusion. Either implement the logic to use eval_iters within the function, or remove the parameter if it's not needed.

Copilot · 2026-02-06T03:31:08Z

pyproject.toml

+include = ["flagscale*"]
+
+[tool.setuptools.package-data]
+"*" = ["*.yaml", "*.yml"]


The package_data configuration in pyproject.toml uses a wildcard "" to match all packages and include ".yaml" and ".yml" files. This is overly broad and will include YAML files from all packages, not just flagscale. Consider being more specific with the package selector, e.g., "flagscale" = [".yaml", "*.yml"] to only include YAML files from the flagscale package.

Suggested change

"*" = ["*.yaml", "*.yml"]

"flagscale" = ["*.yaml", "*.yml"]

Copilot · 2026-02-06T03:31:08Z

flagscale/__init__.py

+    from importlib.metadata import version as get_version
+
+    __version__ = get_version("flagscale")
+except Exception:
+    pass  # Use hardcoded version above


The bare except clause on line 9 catches all exceptions including SystemExit and KeyboardInterrupt, which is generally not recommended. This could mask important errors during import. Consider catching specific exceptions like PackageNotFoundError or ImportError instead, or at minimum use "except Exception:" to avoid catching system-level exceptions.

Suggested change

from importlib.metadata import version as get_version

__version__ = get_version("flagscale")

except Exception:

pass # Use hardcoded version above

from importlib.metadata import PackageNotFoundError, version as get_version

except ImportError:

# importlib.metadata not available; use hardcoded version above

pass

else:

try:

__version__ = get_version("flagscale")

except PackageNotFoundError:

# Package metadata not found; use hardcoded version above

pass

Copilot · 2026-02-06T03:31:08Z

flagscale/cli.py

+def get_action(stop: bool, dryrun: bool, test: bool, query: bool, tune: bool) -> str:
+    """Determine action from flags (mutually exclusive)"""
+    if stop:
+        return "stop"
+    if dryrun:
+        return "dryrun"
+    if test:
+        return "test"
+    if query:
+        return "query"
+    if tune:
+        return "auto_tune"
+    return "run"  # default


The get_action() function doesn't validate that the boolean flags are mutually exclusive. If multiple flags are set (e.g., both --stop and --dryrun), the function will just return the first matching one based on the if-chain order. Consider adding validation to ensure only one action flag is set at a time, or document the priority order clearly.

Copilot · 2026-02-06T03:31:08Z

setup.py

+# Version is defined in pyproject.toml - keep in sync
+FLAGSCALE_VERSION = "1.0.0"


The version is hardcoded in three places (setup.py line 7, pyproject.toml line 10, and flagscale/init.py line 2), which creates a maintenance burden. Consider having setup.py and flagscale/init.py read the version from pyproject.toml to maintain a single source of truth. This could use importlib.metadata or parse pyproject.toml directly.

lxd-cumt

LGTM for training.py

tengqm · 2026-02-06T06:06:39Z

Instead of having users to customize an installation by specifying environment variables, we can leverage the builtin mechanism in pyproject.toml. Say, we have

[project]
dependencies = [
  "foo==1.2.3",
  "core==2.4.0",
]

[project.optional-dependencies]
inference = [
  "vllm==3.4.0",
  "blah==2.3",
]

serve = [
  "flag-scale[inference]",
  "flask==1.0.3"
]

train = [
  "megatron-lm-fl==0.1.0",
  "..."
]

You can easily install flag-scale for different scenarios using the following commands:

pip install flag-scale[inference]
pip install flag-scale[train]
pip install flag-scale[serve]

This is much simpler than writing our own Python code.

tengqm · 2026-02-06T06:11:04Z

PR #1078 was proposed exactly for simplify the installation process using declared dependencies. It can serve as a starting point for further revisions such as a better grouping of dependencies based on usage scenarios; a clearer dependency on the various -FL plugins provided those projects have their own release tags ...

aoyulong · 2026-02-09T10:50:44Z

Instead of having users to customize an installation by specifying environment variables, we can leverage the builtin mechanism in pyproject.toml. Say, we have
[project]
dependencies = [
  "foo==1.2.3",
  "core==2.4.0",
]

[project.optional-dependencies]
inference = [
  "vllm==3.4.0",
  "blah==2.3",
]

serve = [
  "flag-scale[inference]",
  "flask==1.0.3"
]

train = [
  "megatron-lm-fl==0.1.0",
  "..."
]
You can easily install flag-scale for different scenarios using the following commands:

pip install flag-scale[inference]

pip install flag-scale[train]

pip install flag-scale[serve]

This is much simpler than writing our own Python code.

This is our ultimate goal.

Refactor pip install

55b2893

aoyulong requested review from heavyrain-lzy and zhaoyinglia as code owners February 6, 2026 01:57

xmhubj reviewed Feb 6, 2026

View reviewed changes

setup.py

setup(

name="flag_scale",

name="flagscale",

Copy link
Copy Markdown

Collaborator

xmhubj Feb 6, 2026

Choose a reason for hiding this comment

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

same here

aoyulong requested a review from Copilot February 6, 2026 03:23

Copilot started reviewing on behalf of aoyulong February 6, 2026 03:24 View session

Copilot AI reviewed Feb 6, 2026

View reviewed changes

lxd-cumt previously approved these changes Feb 6, 2026

View reviewed changes

Improve the codes

817ae56

aoyulong dismissed lxd-cumt’s stale review via 817ae56 February 6, 2026 07:15

aoyulong added 4 commits February 6, 2026 15:16

Fix the url

e54f59a

Add unit tests for CLI, init, and run commands

d0e67a7

Add pip deps for unit tests

bba31e5

Expand the requriements

b048611

xmhubj approved these changes Feb 9, 2026

View reviewed changes

aoyulong requested a review from Darryl233 February 10, 2026 02:06

Caozhou1995 approved these changes Feb 10, 2026

View reviewed changes

Caozhou1995 merged commit da4dc54 into flagos-ai:main Feb 10, 2026
13 checks passed

	port: int \| None = typer.Option(None, "--port", "-p", help="Server port"),
	port: int \| None = typer.Option(None, "--port", help="Server port"),

		packages=["flagscale"],
		package_dir={"flagscale": "flagscale"},

		# Version is defined in pyproject.toml - keep in sync
		FLAGSCALE_VERSION = "1.0.0"

Conversation

aoyulong commented Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Category

PR Types

PR Description

Uh oh!

xmhubj Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

aoyulong Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

xmhubj Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

xmhubj Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

lxd-cumt left a comment

Choose a reason for hiding this comment

Uh oh!

tengqm commented Feb 6, 2026

Uh oh!

tengqm commented Feb 6, 2026

Uh oh!

aoyulong commented Feb 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

aoyulong commented Feb 6, 2026 •

edited

Loading