Don't Install Tableau API on arm64 #218

mzappitello · 2024-01-08T21:05:56Z

The tableauhyperapi module is unavailable on mac with arm64 processors.

Update poetry to take advantage of dependency groups.
Add a tableau dependency group that can be excluded when installing dependencies.
Update python_src to fallback with error messages when using the tableau subdir that creates hyperfiles fails to import everything.
Use new BUILDPLATFORM arg in dockerfile to build without tableau deps on an arm64 platform.
Add an export for the BUILDPLATFORM variable to .envrc and pass it through via docker-compose when building the lamp py images.

Asana Task: <asana_ticket_url>

mzappitello · 2024-01-08T21:06:35Z

.envrc

this is passed to docker compose, which passes it to the dockerfile, which uses it to install tableau deps or not.

mzappitello · 2024-01-08T21:07:02Z

docker-compose.yml

+      args:
+        BUILDPLATFORM: $BUILDPLATFORM


pass arg to dockerfile which is used to install poetry or not.

mzappitello · 2024-01-08T21:07:38Z

python_src/Dockerfile

+# Tableau dependencies for arm64 cannot be resolved (since salesforce doesn't
+# support them yet). For that buildplatform build without those dependencies
+ARG BUILDPLATFORM
+RUN echo "Installing python dependencies for ${BUILDPLATFORM}."
+RUN if [ "$BUILDPLATFORM" = "arm64" ]; then \
+    poetry install --without tableau --no-interaction --no-ansi -v ;\
+    else poetry install --no-interaction --no-ansi -v ;\
+    fi


if its an arm64 build platform, install without the tableau dep group.

mzappitello · 2024-01-08T21:08:43Z

python_src/pyproject.toml

@@ -27,10 +27,15 @@ psutil = "^5.9.1"
 schedule = "^1.1.0"
 alembic = "^1.10.2"
 types-pytz = "^2023.3.0.1"
+
+[tool.poetry.group.tableau]


add dependency group for tableau dependencies. its not optional, meaning a user will have to change behavior to build without it.

mzappitello · 2024-01-08T21:09:16Z

python_src/src/lamp_py/tableau/__init__.py

@@ -1 +1,21 @@
-"""Utilites for Interacting with Tableau and Hyper files"""
+"""Utilities for Interacting with Tableau and Hyper files"""


update the tableau init file to catch exceptions when the tableau modues arent found.

mzappitello · 2024-01-08T21:10:03Z

python_src/src/lamp_py/performance_manager/pipeline.py

-from lamp_py.tableau.pipeline import start_parquet_updates
+from lamp_py.tableau import start_parquet_updates


this is the only part i don't like. if we import directly from the file we're doing to get the import error and it felt like catch that error should be left to the subdir.

The tableauhyperapi module is unavailable on mac with arm64 processors. * Update poetry to take advantage of dependency groups. * Add a tableau dependency group that can be excluded when installing dependencies. * Update python_src to fallback with error messages when using the tableau subdir that creates hyperfiles fails to import everything. * Use new `BUILDPLATFORM` arg in dockerfile to build without tableau deps on an arm64 platform. * Add an export for the BUILDPLATFORM variable to .envrc and pass it through via docker-compose when building the lamp py images.

rymarczy

This all looks like it's functioning as expected for me locally.

I think we need to add some documentation regarding why/how we are doing this in a README file. We are also missing a README for the "tableau" applications, which is my oversight when originally adding that functionality.

@paulswartz Could you also please review this approach and see if you can find any potential issues with this approach? Are any other TID apps doing anything similar with a different approach?

paulswartz · 2024-01-10T15:59:53Z

.envrc

+
+# used in dockercompose and dockerfile 
+export BUILDPLATFORM=$(uname -m)


question: Do you need to set this? At least according to the Docker documentation this should be provided already.

i tried this out locally based on this same documentation and found that the value evaluated to an empty string when I ran on my machine.

This diff worked for me without needing to set any additional variables:

diff --git a/python_src/Dockerfile b/python_src/Dockerfile index 06a3e1e..40d69a6 100644 --- a/python_src/Dockerfile +++ b/python_src/Dockerfile @@ -25,9 +25,9 @@ COPY pyproject.toml pyproject.toml # Tableau dependencies for arm64 cannot be resolved (since salesforce doesn't # support them yet). For that buildplatform build without those dependencies -ARG BUILDPLATFORM -RUN echo "Installing python dependencies for ${BUILDPLATFORM}." -RUN if [ "$BUILDPLATFORM" = "arm64" ]; then \ +ARG TARGETARCH BUILDPLATFORM TARGETPLATFORM +RUN echo "Installing python dependencies for build: ${BUILDPLATFORM} target: ${TARGETPLATFORM}" +RUN if [ "$TARGETARCH" = "arm64" ]; then \ poetry install --without tableau --no-interaction --no-ansi -v ;\ else poetry install --no-interaction --no-ansi -v ;\ fi

paulswartz · 2024-01-10T16:02:58Z

python_src/src/lamp_py/tableau/__init__.py

@@ -1 +1,29 @@
-"""Utilites for Interacting with Tableau and Hyper files"""
+"""Utilities for Interacting with Tableau and Hyper files"""
+


suggestion(non-blocking):

try: import .pipeline as pipeline except ModuleNotFoundError: pipeline = None def start_parquet_updates(db_manager: DatabaseManager) -> None if pipeline is None: logging.exception(...) else: pipeline.start_parquet_updates(db_manager)

oh. i like this much better.

* Change some of the details in the tableau __init__ file. * No need to pass in build platform arg. remove from direnv and docker-compose.

rymarczy

LGTM, working locally for me.

mzappitello commented Jan 8, 2024

View reviewed changes

mzappitello force-pushed the tableau_build_args branch from 89662bd to 39a5843 Compare January 9, 2024 02:39

rymarczy requested changes Jan 10, 2024

View reviewed changes

rymarczy requested a review from paulswartz January 10, 2024 14:44

paulswartz reviewed Jan 10, 2024

View reviewed changes

mzappitello requested a review from paulswartz January 10, 2024 20:49

mzappitello force-pushed the tableau_build_args branch 2 times, most recently from 20fcbba to fa19ad6 Compare January 11, 2024 15:26

mzappitello added 2 commits January 11, 2024 11:08

Improvement from PR 218 - Squash on Merge

5406d97

* Change some of the details in the tableau __init__ file. * No need to pass in build platform arg. remove from direnv and docker-compose.

Add Tableau dir README.md

1bdb816

mzappitello force-pushed the tableau_build_args branch from fa19ad6 to 1bdb816 Compare January 11, 2024 16:08

rymarczy self-requested a review January 11, 2024 17:45

rymarczy approved these changes Jan 11, 2024

View reviewed changes

mzappitello merged commit 471e2d2 into main Jan 11, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't Install Tableau API on arm64 #218

Don't Install Tableau API on arm64 #218

mzappitello commented Jan 8, 2024

mzappitello Jan 8, 2024

mzappitello Jan 8, 2024

mzappitello Jan 8, 2024

mzappitello Jan 8, 2024

mzappitello Jan 8, 2024

mzappitello Jan 8, 2024

rymarczy left a comment

paulswartz Jan 10, 2024

mzappitello Jan 10, 2024

paulswartz Jan 10, 2024

paulswartz Jan 10, 2024

mzappitello Jan 10, 2024

rymarczy left a comment

		@@ -1 +1,21 @@
		"""Utilites for Interacting with Tableau and Hyper files"""
		"""Utilities for Interacting with Tableau and Hyper files"""

		from lamp_py.tableau.pipeline import start_parquet_updates
		from lamp_py.tableau import start_parquet_updates


		# used in dockercompose and dockerfile
		export BUILDPLATFORM=$(uname -m)

		@@ -1 +1,29 @@
		"""Utilites for Interacting with Tableau and Hyper files"""
		"""Utilities for Interacting with Tableau and Hyper files"""

Don't Install Tableau API on arm64 #218

Don't Install Tableau API on arm64 #218

Conversation

mzappitello commented Jan 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rymarczy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rymarczy left a comment

Choose a reason for hiding this comment