feat: Migrate HTCondor submission to native Python API (Fixes #336) ; - #510
Open
gluonparticle wants to merge 3 commits into
Open
feat: Migrate HTCondor submission to native Python API (Fixes #336) ;#510gluonparticle wants to merge 3 commits into
gluonparticle wants to merge 3 commits into
Conversation
- Replaced legacy subprocess shell calls with native htcondor.Submit objects. - Implemented Schedd.transaction() for atomic, reliable job submission. - Patched determine_os() to prevent FileNotFoundError on non-RHEL systems. - Added mock-based unit tests to verify retry logic and error handling.
- Moved 'import htcondor' inside functions to prevent ModuleNotFoundError in lightweight CI environments that lack grid bindings.
Contributor
|
Hi @gluonparticle, this is cool :) Can you explain what is the reason behind removing the comments? |
Author
Hi @kjvbrt , thanks! Glad you like the native API approach. To be honest. as I was testing CI fixes, I just got a bit carried away with 'cleaning up' the file. As I was trying to pursue a minimalistic approach on the format , I endeed up removing some comments which might hinder code readability. Looking at the diff, I realize I might've made things harder to review. My bad! Please let me know if I should revert those comments and if you need anything else from my side. Thanks! ; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR migrates the batch submission system from legacy shell-scripting to the native HTCondor Python API. This fixes #336 while improves stability, security, and portability across different operating systems.
Key Changes:
Native API: Switched to htcondor.Submit and htcondor.Schedd for job dispatching.
Security (Critical): Eliminated a Shell Injection risk by removing subprocess calls with shell=True. Sample names and configuration strings are now handled as Python objects, preventing potential command execution through malicious input.
Atomic Transactions: Wrapped submissions in schedd.transaction() for ensuring grid state consistency.
Portability Fix: Patched a hardcoded OS check that caused FileNotFoundError on non-RHEL systems (Arch, Ubuntu, macOS).
Verification & Tests:
I have verified the logic locally using the included mock suite on Arch Linux and also included a local regression test in tests/unit/batch/.
Note:
Since I developed this on a non-CERN environment, I included a mock-based test suite to verify the retry logic and transactional flow. If you prefer to keep the tests/ directory strictly for integration tests, I am happy to move these to a gist or remove them before merging.
I am happy to spin up an AlmaLinux container to verify the bindings against the specific glibc version used in the Key4hep stack ;