Skip to content

Fix/config path - #124

Merged
r-krishna merged 3 commits into
productionfrom
fix/config_path
Dec 20, 2025
Merged

Fix/config path#124
r-krishna merged 3 commits into
productionfrom
fix/config_path

Conversation

@r-krishna

Copy link
Copy Markdown
Collaborator

This PR updates the RF3 CLI to enable running RF3 from the command line when pip installed while also maintaining the ability to use the CLI with a dev installation. It uses the same pattern used for the same thing in RFD3.

import pandas as pd
from beartype.typing import Any
from lightning_fabric.utilities import rank_zero_only
from lightning.fabric.utilities import rank_zero_only

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

this is just a more modern import pattern for fabric which has been subsumed into lightning.

@r-krishna
r-krishna merged commit 1184ddb into production Dec 20, 2025
5 checks passed
@r-krishna
r-krishna deleted the fix/config_path branch December 20, 2025 19:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR enables the RF3 CLI to work correctly when the package is pip-installed while maintaining compatibility with development installations. The changes follow a similar pattern used in RFD3 for handling both installation modes.

Key changes:

  • Updated import statements from lightning_fabric to lightning.fabric for consistency with the rest of the codebase
  • Modified RF3 CLI config path resolution to detect and handle both installed and development modes
  • Corrected pyproject.toml configuration by moving local-scheme to the appropriate section

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/foundry/utils/logging.py Updated Lightning import to use modern lightning.fabric package path
src/foundry/utils/ddp.py Updated Lightning import to use modern lightning.fabric package path
pyproject.toml Moved local-scheme configuration from hooks section to version section (correct location per hatch-vcs)
models/rf3/src/rf3/inference.py Changed config path from PROJECT_ROOT-based to relative path resolution
models/rf3/src/rf3/cli.py Implemented dual-mode config path detection that checks for installed mode first, then falls back to development mode

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

Comment on lines +19 to +21
_config_path = os.path.join(
os.path.dirname(os.path.dirname(os.path.dirname(__file__))), "configs"
)

Copilot AI Dec 20, 2025

Copy link

Choose a reason for hiding this comment

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

The config path resolution differs from the cli.py pattern and doesn't properly handle both installed and development modes. While the primary CLI path (via cli.py) works because it initializes Hydra before calling run_inference, direct execution would only work in development mode.

For consistency and robustness, consider matching the cli.py pattern: check if configs exist as a sibling directory first (for installed mode), then fall back to the development path. This would make both entry points handle installation modes consistently.

Suggested change
_config_path = os.path.join(
os.path.dirname(os.path.dirname(os.path.dirname(__file__))), "configs"
)
# Prefer a sibling 'configs' directory (installed mode); fall back to the
# development layout where 'configs' lives three levels up.
_sibling_configs = os.path.join(os.path.dirname(__file__), "configs")
_dev_configs = os.path.join(
os.path.dirname(os.path.dirname(os.path.dirname(__file__))), "configs"
)
_config_path = _sibling_configs if os.path.isdir(_sibling_configs) else _dev_configs

Copilot uses AI. Check for mistakes.
Comment thread models/rf3/src/rf3/cli.py
Comment on lines +29 to +38

# Check if we're in installed mode (configs are sibling to this file)
# or development mode (configs are ../../../configs)
if (rf3_file_dir / "configs").exists():
# Installed mode
config_path = str(rf3_file_dir / "configs")
else:
# Development mode
rf3_package_dir = rf3_file_dir.parent.parent # Go up to models/rf3/
config_path = str(rf3_package_dir / "configs")

Copilot AI Dec 20, 2025

Copy link

Choose a reason for hiding this comment

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

The order of checking installed vs development mode is reversed compared to the RFD3 implementation pattern. This checks for installed mode first, which could cause issues if someone has the package installed and is also working in a development environment - it would use the potentially stale installed configs instead of the development ones.

Consider following the RFD3 pattern: check for the development path first, then fall back to the installed path. This ensures that when working in development mode, the development configs are always used.

Suggested change
# Check if we're in installed mode (configs are sibling to this file)
# or development mode (configs are ../../../configs)
if (rf3_file_dir / "configs").exists():
# Installed mode
config_path = str(rf3_file_dir / "configs")
else:
# Development mode
rf3_package_dir = rf3_file_dir.parent.parent # Go up to models/rf3/
config_path = str(rf3_package_dir / "configs")
rf3_package_dir = rf3_file_dir.parent.parent # Go up to models/rf3/ in development
# Check if we're in development mode (configs are ../../../configs)
# or installed mode (configs are sibling to this file)
if (rf3_package_dir / "configs").exists():
# Development mode
config_path = str(rf3_package_dir / "configs")
else:
# Installed mode
config_path = str(rf3_file_dir / "configs")

Copilot uses AI. Check for mistakes.
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