Skip to content

feat: UN-2166 Passed optional container name from backend to runner#1158

Merged
ritwik-g merged 3 commits into
mainfrom
feat/container-name-passed-to-runner-from-backend
Feb 28, 2025
Merged

feat: UN-2166 Passed optional container name from backend to runner#1158
ritwik-g merged 3 commits into
mainfrom
feat/container-name-passed-to-runner-from-backend

Conversation

@chandrasekharan-zipstack

Copy link
Copy Markdown
Contributor

What

  • Passed container name from backend to runner
  • MINOR: Renamed run_id to file_execution_id in parts concerning WF execution

Why

  • Previously container name was logged in the backend and a different one was generated and used by the runner which resulted in confusion during debugging and tracing
  • Ensuring a single container name is logged for a run and using the same throughout

How

  • Passed via the HTTP call to the runner

Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)

  • No, changes made with defaults - tests and and all associated code is updated

Dependencies Versions

  • Added unstract-core to unstract-tool-sandbox

Notes on Testing

  • Tested a WF run locally and it works against docker

Screenshots

image

Checklist

I have read and understood the Contribution Guidelines.

…a private lib that's added, the lockfile can't be resolved and pushed

@muhammad-ali-e muhammad-ali-e left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM overall, left few NITs

Comment thread runner/src/unstract/runner/clients/docker.py
Comment thread unstract/core/src/unstract/core/utilities.py Outdated
@github-actions

Copy link
Copy Markdown
Contributor
filepath function $$\textcolor{#23d18b}{\tt{passed}}$$ SUBTOTAL
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_logs}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_cleanup}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_cleanup\_skip}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_client\_init}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_image\_exists}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_image}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_container\_run\_config}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_container\_run\_config\_without\_mount}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_run\_container}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{TOTAL}}$$ $$\textcolor{#23d18b}{\tt{9}}$$ $$\textcolor{#23d18b}{\tt{9}}$$

@sonarqubecloud

Copy link
Copy Markdown

Comment thread docker/scripts/pdm-lock-gen/pdm-lock.sh
Comment thread backend/workflow_manager/workflow_v2/execution.py
Comment thread runner/src/unstract/runner/runner.py

@ritwik-g ritwik-g left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I feel like you have combined way too many changes in to this one PR making it a bit difficult to review. If the PR only had the container name related changes it would have been easier to review. Keeping the PR with single responsibility help the reviewers. Especially with some items like file_name removal the new retry count logic in the name. With all these changes am finding it difficult to review the PR.

When we are not hard pressed for time this might be something passable. But now since we are trying to take this change quickly in the other changes are becoming an unnecessary burden. It takes longer to review and the chances for bugs also increases.

Anyway since @muhammad-ali-e has approved I would consider this relatively okay. For some items outside runner I don't have enough context to understand the changes. So I am not thoroughly reviewing those.

But I have left one comment which needs to be addressed. Please check that

@ritwik-g ritwik-g merged commit c4fb6b8 into main Feb 28, 2025
@ritwik-g ritwik-g deleted the feat/container-name-passed-to-runner-from-backend branch February 28, 2025 14:02
pk-zipstack pushed a commit that referenced this pull request Aug 20, 2025
…1158)

* UN-2166 Passed optional container name from backend to runner

* Removed tool-sandbox from lockfile automation script - since core is a private lib that's added, the lockfile can't be resolved and pushed

* minor: Addressed review comments for renaming a variable
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