Skip to content

Conversation

@slimfrkha
Copy link

@slimfrkha slimfrkha commented Aug 25, 2025

What does this PR do?

Solve #3201

Problem

The existing license check hook scans all directories recursively from a single root directory, which causes issues in local development environments:

  • Virtual environments (.venv, venv/) get scanned and fail license checks
  • No easy way to exclude common build/cache directories without hardcoding exclusions
  • Different behavior between local development (with venvs) and CI/CD (clean environment)

Solution

Modified the check_license.py script to accept multiple target directories instead of a single root directory with exclusions.

Design & Code Changes

Changed argument from --directory to --directories

  • Now accepts multiple Path arguments using nargs="+"
  • Allows specifying exactly which directories to scan
  • in local mode: --directories examples recipe scripts tests verl setup.py
  • in github workflow: --directories .

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request improves the flexibility of the license check hook by allowing multiple directories to be specified. This is a good improvement for local development environments. My review identifies a critical issue where the script does not correctly handle individual file paths (like setup.py) passed as arguments, which would cause them to be skipped during the license check. I've provided a code suggestion to fix this by handling both directory and file paths correctly.

@vermouth1992 vermouth1992 merged commit 11a43b6 into volcengine:main Aug 25, 2025
9 checks passed
PopSoda2002 pushed a commit to PopSoda2002/verl that referenced this pull request Aug 26, 2025
### What does this PR do?

Solve volcengine#3201

#### Problem
The existing license check hook scans all directories recursively from a
single root directory, which causes issues in local development
environments:

* Virtual environments (`.venv`, `venv/`) get scanned and fail license
checks
* No easy way to exclude common build/cache directories without
hardcoding exclusions
* Different behavior between local development (with venvs) and CI/CD
(clean environment)

#### Solution
Modified the `check_license.py` script to accept multiple target
directories instead of a single root directory with exclusions.

### Design & Code Changes
Changed argument from `--directory` to `--directories`
* Now accepts multiple `Path` arguments using `nargs="+"`
* Allows specifying exactly which directories to scan
* in local mode: `--directories examples recipe scripts tests verl
setup.py`
* in github workflow: `--directories .`
yellowbee686 pushed a commit to yellowbee686/verl that referenced this pull request Aug 27, 2025
### What does this PR do?

Solve volcengine#3201

#### Problem
The existing license check hook scans all directories recursively from a
single root directory, which causes issues in local development
environments:

* Virtual environments (`.venv`, `venv/`) get scanned and fail license
checks
* No easy way to exclude common build/cache directories without
hardcoding exclusions
* Different behavior between local development (with venvs) and CI/CD
(clean environment)

#### Solution
Modified the `check_license.py` script to accept multiple target
directories instead of a single root directory with exclusions.

### Design & Code Changes
Changed argument from `--directory` to `--directories`
* Now accepts multiple `Path` arguments using `nargs="+"`
* Allows specifying exactly which directories to scan
* in local mode: `--directories examples recipe scripts tests verl
setup.py`
* in github workflow: `--directories .`
cczitong123 pushed a commit to cczitong123/verl that referenced this pull request Sep 5, 2025
### What does this PR do?

Solve volcengine#3201

#### Problem
The existing license check hook scans all directories recursively from a
single root directory, which causes issues in local development
environments:

* Virtual environments (`.venv`, `venv/`) get scanned and fail license
checks
* No easy way to exclude common build/cache directories without
hardcoding exclusions
* Different behavior between local development (with venvs) and CI/CD
(clean environment)

#### Solution
Modified the `check_license.py` script to accept multiple target
directories instead of a single root directory with exclusions.

### Design & Code Changes
Changed argument from `--directory` to `--directories`
* Now accepts multiple `Path` arguments using `nargs="+"`
* Allows specifying exactly which directories to scan
* in local mode: `--directories examples recipe scripts tests verl
setup.py`
* in github workflow: `--directories .`
DDVD233 pushed a commit to DDVD233/mirl that referenced this pull request Sep 5, 2025
### What does this PR do?

Solve volcengine#3201

#### Problem
The existing license check hook scans all directories recursively from a
single root directory, which causes issues in local development
environments:

* Virtual environments (`.venv`, `venv/`) get scanned and fail license
checks
* No easy way to exclude common build/cache directories without
hardcoding exclusions
* Different behavior between local development (with venvs) and CI/CD
(clean environment)

#### Solution
Modified the `check_license.py` script to accept multiple target
directories instead of a single root directory with exclusions.

### Design & Code Changes
Changed argument from `--directory` to `--directories`
* Now accepts multiple `Path` arguments using `nargs="+"`
* Allows specifying exactly which directories to scan
* in local mode: `--directories examples recipe scripts tests verl
setup.py`
* in github workflow: `--directories .`
WncFht pushed a commit to WncFht/verl that referenced this pull request Oct 10, 2025
### What does this PR do?

Solve volcengine#3201

#### Problem
The existing license check hook scans all directories recursively from a
single root directory, which causes issues in local development
environments:

* Virtual environments (`.venv`, `venv/`) get scanned and fail license
checks
* No easy way to exclude common build/cache directories without
hardcoding exclusions
* Different behavior between local development (with venvs) and CI/CD
(clean environment)

#### Solution
Modified the `check_license.py` script to accept multiple target
directories instead of a single root directory with exclusions.

### Design & Code Changes
Changed argument from `--directory` to `--directories`
* Now accepts multiple `Path` arguments using `nargs="+"`
* Allows specifying exactly which directories to scan
* in local mode: `--directories examples recipe scripts tests verl
setup.py`
* in github workflow: `--directories .`
techkang pushed a commit to techkang/verl that referenced this pull request Oct 31, 2025
### What does this PR do?

Solve volcengine#3201

#### Problem
The existing license check hook scans all directories recursively from a
single root directory, which causes issues in local development
environments:

* Virtual environments (`.venv`, `venv/`) get scanned and fail license
checks
* No easy way to exclude common build/cache directories without
hardcoding exclusions
* Different behavior between local development (with venvs) and CI/CD
(clean environment)

#### Solution
Modified the `check_license.py` script to accept multiple target
directories instead of a single root directory with exclusions.

### Design & Code Changes
Changed argument from `--directory` to `--directories`
* Now accepts multiple `Path` arguments using `nargs="+"`
* Allows specifying exactly which directories to scan
* in local mode: `--directories examples recipe scripts tests verl
setup.py`
* in github workflow: `--directories .`
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