Skip to content

Conversation

@lucasbazan
Copy link

Description

Refactored research/delf/delf/python/datasets/sfm120k/dataset_download.py to remove dependencies on OS-specific shell commands invoked via os.system.

Changes implemented:

  1. Download: Replaced os.system('wget ...') with urllib.request.urlretrieve.
  2. Extraction: Replaced os.system('tar ...') with the native tarfile module.
  3. Cleanup: Replaced os.system('rm ...') with os.remove.
  4. Symlinks: Replaced os.system('ln -s ...') with os.symlink.

Motivation and Context

  • Portability: The previous implementation failed on Windows environments because it relied on wget, rm, and ln binaries which are not present by default. Using native Python libraries ensures cross-platform compatibility.
  • Security: Removing os.system eliminates the risk of shell injection vulnerabilities and is considered a best practice for file operations.

Dependencies

  • No new external dependencies. Uses standard libraries: urllib, tarfile, os, shutil.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Documentation update
  • TensorFlow 2 migration
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • A new research paper code implementation
  • Other (Specify)

Tests

I manually verified the changes by running the script locally:

  1. Verified that the dataset files are downloaded correctly via urllib.
  2. Verified that tarfile correctly extracts the archives to the target directory.
  3. Verified that symlinks are created successfully (on systems that support it).

Checklist

@google-cla
Copy link

google-cla bot commented Jan 28, 2026

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

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.

1 participant