Skip to content

fix: use os.path.join for robust path handling in llm_op#1389

Closed
RithamSharma wants to merge 1 commit into
dora-rs:mainfrom
RithamSharma:fix/path-concatenation-bug
Closed

fix: use os.path.join for robust path handling in llm_op#1389
RithamSharma wants to merge 1 commit into
dora-rs:mainfrom
RithamSharma:fix/path-concatenation-bug

Conversation

@RithamSharma
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Collaborator

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

current_directory = os.path.dirname(current_file_path)

path = current_directory + "object_detection.py"
path = os.path.join(current_directory,"object_detection.py") # ← FIXED
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please remove the # ← FIXED comment.

@heyong4725
Copy link
Copy Markdown
Collaborator

Hi @RithamSharma — thanks for spotting this bug and posting the fix. Closing this PR but the fix is being rescued and credited to you in #1837.

Your analysis was right: current_directory + "object_detection.py" is missing the path separator, and os.path.join is the correct idiom. I lifted that one-line change verbatim, with two tiny cosmetic cleanups before merging:

  1. Dropped the trailing # ← FIXED annotation (debug marker, doesn't belong in the example file)
  2. Added PEP 8 spacing after the comma in os.path.join(current_directory, "object_detection.py")

You're credited via Co-Authored-By on the commit, so when #1837 lands your name will be on it as a co-author. The PR description also links back to your original PR explicitly.

Two notes for next time, both small:

  • For trivial fixes, don't add inline markers like # ← FIXED in the patched line — they're useful while you're staging the change locally, but they end up shipped to other readers if left in. A clean diff is just the corrected line.
  • PEP 8 wants a space after commas in function calls: f(a, b) not f(a,b). The ruff / black configs in this repo will flag this automatically if you have them set up locally.

Thanks again — looking forward to your next PR.

@heyong4725 heyong4725 closed this May 16, 2026
heyong4725 added a commit that referenced this pull request May 16, 2026
The __main__ block of `examples/python-operator-dataflow/llm_op.py`
(a developer-mode entry point for exercising the operator directly,
distinct from the operator import path used by the dora dataflow)
built a path via string concatenation:

    path = current_directory + "object_detection.py"

This produces `<dir>object_detection.py` with no separator, so
`python llm_op.py` would always fail to open the file unless it
happened to be in a directory whose name ended in a path separator.

Rescue of #1389 (RithamSharma) which identified the bug and the
correct fix (os.path.join). Reimplemented with PEP 8 spacing and
without the trailing debug comment so the example stays clean.

Co-Authored-By: RithamSharma <RithamSharma@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
trunk-io Bot pushed a commit that referenced this pull request May 17, 2026
#1837)

The __main__ block of `examples/python-operator-dataflow/llm_op.py`
(a developer-mode entry point for exercising the operator directly,
distinct from the operator import path used by the dora dataflow)
built a path via string concatenation:

    path = current_directory + "object_detection.py"

This produces `<dir>object_detection.py` with no separator, so
`python llm_op.py` would always fail to open the file unless it
happened to be in a directory whose name ended in a path separator.

Rescue of #1389 (RithamSharma) which identified the bug and the
correct fix (os.path.join). Reimplemented with PEP 8 spacing and
without the trailing debug comment so the example stays clean.

Co-authored-by: RithamSharma <RithamSharma@users.noreply.github.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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