Skip to content

Conversation

@brosenberg42
Copy link
Member

@brosenberg42 brosenberg42 commented Mar 11, 2025

Copy link
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @brosenberg42)


a discussion (no related file):
Please update openmpf-docker/components/cli_runner/tests/Dockerfile : FROM python:3.8-alpine


workflow_manager/Dockerfile line 55 at r1 (raw file):

    apt-get update; \
    apt-get install --no-install-recommends -y python3.12-venv; \
    python3.12 -m ensurepip --upgrade --default-pip; \

This code to install python 3.12 appears in a lot of Dockerfiles. Not all of them do:

python3.12 -m ensurepip --upgrade --default-pip;

For example, the Java executor and CPP executor. The Subject Component executor updates pip in the venv later on. Is there a reason for the inconsistency?

Does it have something to do with whether the Docker services need to install modules through pip?


subject-components/python/Dockerfile line 85 at r1 (raw file):

eot

RUN --mount=from=openmpf_build,source=/home/mpf/openmpf-projects/openmpf-python-component-sdk/detection,target=/tmp/python,rw \

Why add rw now?

Copy link
Member Author

@brosenberg42 brosenberg42 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 4 of 7 files reviewed, 3 unresolved discussions (waiting on @brosenberg42 and @jrobble)


a discussion (no related file):

Previously, jrobble (Jeff Robble) wrote…

Please update openmpf-docker/components/cli_runner/tests/Dockerfile : FROM python:3.8-alpine

Done.


subject-components/python/Dockerfile line 85 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Why add rw now?

Pip now writes files.


workflow_manager/Dockerfile line 55 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

This code to install python 3.12 appears in a lot of Dockerfiles. Not all of them do:

python3.12 -m ensurepip --upgrade --default-pip;

For example, the Java executor and CPP executor. The Subject Component executor updates pip in the venv later on. Is there a reason for the inconsistency?

Does it have something to do with whether the Docker services need to install modules through pip?

The other language don't use pip and WFM doesn't use a venv.

Copy link
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

@jrobble reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @brosenberg42)


a discussion (no related file):

openmpf-docker/components/cli_runner/mpf_python_runner.py uses pkg_resources:

entry_points = pkg_resources.get_entry_map(distribution_name, 'mpf.exported_component')

Is that something we should change?

If you want to leave it, I'm fine with that. We install setuptools in openmpf-docker/components/python/Dockerfile.

Copy link
Member Author

@brosenberg42 brosenberg42 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 7 of 8 files reviewed, 1 unresolved discussion (waiting on @brosenberg42 and @jrobble)


a discussion (no related file):

Previously, jrobble (Jeff Robble) wrote…

openmpf-docker/components/cli_runner/mpf_python_runner.py uses pkg_resources:

entry_points = pkg_resources.get_entry_map(distribution_name, 'mpf.exported_component')

Is that something we should change?

If you want to leave it, I'm fine with that. We install setuptools in openmpf-docker/components/python/Dockerfile.

I updated it to use importlib.metadata

Copy link
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

@jrobble reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @brosenberg42)

Copy link
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @brosenberg42)

@brosenberg42 brosenberg42 merged commit 57787a4 into develop Sep 10, 2025
2 checks passed
@brosenberg42 brosenberg42 deleted the feat/py3.12 branch September 10, 2025 17:20
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