Skip to content

Add Python package definition#17

Open
carlthome wants to merge 3 commits intov-iashin:mainfrom
carlthome:add-package-definition
Open

Add Python package definition#17
carlthome wants to merge 3 commits intov-iashin:mainfrom
carlthome:add-package-definition

Conversation

@carlthome
Copy link
Contributor

@carlthome carlthome commented Oct 21, 2025

What?

  1. Add a basic pyproject.toml next to the conda environment, to make the Synchformer code pip install:able
  2. Add a GitHub Actions workflow to test that pip install works
  3. Add two lightweight unit tests (mostly to make sure import works after install)

Tested to work here

Why?

After this change, it will be possible to run

pip install ./Synchformer
python -c "import Synchformer.data"

and

python -m Synchformer.example --help    
usage: example.py [-h] --exp_name EXP_NAME --vid_path VID_PATH [--offset_sec OFFSET_SEC] [--v_start_i_sec V_START_I_SEC] [--device DEVICE]

options:
  -h, --help            show this help message and exit
  --exp_name EXP_NAME   In a format: xx-xx-xxTxx-xx-xx
  --vid_path VID_PATH   A path to .mp4 video
  --offset_sec OFFSET_SEC
  --v_start_i_sec V_START_I_SEC
  --device DEVICE

which is a nice convenience for testing this out. I've made sure to not change any existing code to make this work (such that things behave the same as before).

@carlthome carlthome force-pushed the add-package-definition branch from 16983ff to 763728b Compare October 23, 2025 07:21
@carlthome carlthome force-pushed the add-package-definition branch from 763728b to 053b964 Compare October 23, 2025 07:25
@carlthome carlthome marked this pull request as ready for review October 23, 2025 08:23
@v-iashin
Copy link
Owner

Thank you! That's amazing - good description! I always wanted to make it a package.

A few questions.

First, when it comes to the use of a package, I'd like to do pip install x (or from GitHub) and then do something like:

from synchformer import Synchformer

s = Synchformer(ckpt_path_or_exp='XX-XX-XXT... or xxx.pt') 

offset = s(video='vid_wo_offset.mp4')
# `offset` output is `0.0` (a top-1 logit) or `logits` or `softmax(logits)`

How far away are we from this?

Second, I noticed you're testing only 'fake' inputs. Could you include a video, e.g., from _repo_assets, and test the output (e.g. top logit is within tol from some hard-coded value for that video, e.g. 11.5469)?

Third, av is missing from the list of packages as it is a crucial depependency. I realise that av is also a source of problems (#11) but it should be there, shouldn't it?

@dakl
Copy link

dakl commented Nov 28, 2025

@v-iashin one option here could be to get this in and then potentially follow up with the changes that you refer to, which I agree would be nice. As a start however, being able to avoid

    import sys
    sys.path.insert(0, "/app/Synchformer/")
    from utils.utils import which_ffmpeg

would be a win :D

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.

3 participants