facebookresearch / facebookresearch/sam2

Clarify build_sam2 config_file contract and support external config dirs

Open
#758 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Jupyter Notebook
Stars
19.9k
Forks
2.5k
PR merge metrics
No merged PRs in 30d

Description

`build_sam2()` currently accepts an argument named `config_file` and passes it directly to Hydra as `config_name`:

https://github.com/facebookresearch/sam2/blob/2b90b9f5ceec907a1c18123530e92e794ad901a4/sam2/build_sam.py#L69-L92

```python
cfg = compose(config_name=config_file, overrides=hydra_overrides_extra)
```

SAM2 initializes Hydra with the `sam2` config module:

https://github.com/facebookresearch/sam2/blob/2b90b9f5ceec907a1c18123530e92e794ad901a4/sam2/__init__.py

```python
initialize_config_module("sam2", version_base="1.2")
```

That means `config_file` is not a generic filesystem path. It is a Hydra config name/path resolved inside Hydra's initialized config search path, for example:

```python
"configs/sam2.1/sam2.1_hiera_l.yaml"
```

This has confused users who pass absolute filesystem paths like:

```python
"/kaggle/working/models/sam2.1_hiera_l.yaml"
```

Related reports:

- facebookresearch/sam2#544
- facebookresearch/hydra#3033
- facebookresearch/hydra#3045

From Hydra's perspective, this is expected: `compose(config_name=...)` resolves a config name from the configured search path. It does not mean "load this arbitrary file path."

I think SAM2 should clarify this contract and, if external config files are supported, expose that explicitly. A clean API could accept an additional optional config directory/search path argument, then initialize or extend Hydra's config search path appropriately. Users would pass something like:

```python
build_sam2(
config_file="sam2.1_hiera_l.yaml",
config_dir="/kaggle/working/models",
ckpt_path=...,
)
```

or similar.

The important point is that `config_file` should remain a config name resolved inside the search path, while `config_dir` or an equivalent argument tells SAM2 where external configs live.

This avoids relying on the current double-slash workaround and gives users a clearer mental model:

- `config_dir`: where to search
- `config_file` / `config_name`: what config to compose from that search path

Can you clarify the intended contract for `build_sam2(config_file=...)` and consider adding an explicit external config directory option if loading user-provided config files is supported?

Contributor guide

Open the contributing guide

Research direction

Start with sam2/build_sam.py, especially build_sam2() and its compose(config_name=...) call, then read sam2/__init__.py to understand Hydra initialization. Check the linked Hydra reports and existing configuration layout before choosing how an external search path should be exposed. Done means the config_file contract is documented and, if supported, an explicit external config directory works without the double-slash workaround.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
tooling
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.