dev: simplify #18

sirupsen · 2022-05-10T14:38:01Z

The dev setup right now is pretty unreliable. I'm trying to vastly simplify it and make it more robust. I'm commenting inline why I've made certain changes.

sirupsen · 2022-05-10T14:45:59Z

README.md


 It's highly recommended that all involved columns are indexed.

+## Development Setup


It's too easy to miss it otherwise. Let's just have one big README.

Are you sure that we only want to have doc-based instructions for a setup? Check DAT-3098

I would love a script, the reason for a doc-based approach for now is (1) these currently take too long to be one script, (2) too hard to test, because it takes too long to test, and (3) the existing script didn't work (because of (2)). We are not mature enough for a script, unfortunately.

sirupsen · 2022-05-10T14:46:23Z

dev/prepare_db.pql


    run_sql("RM @~/ratings.csv.gz")
-    run_sql("PUT file://ml-25m/ratings.csv @~")
+    run_sql("PUT file://dev/ml-25m/ratings.csv @~")


The commands in the README straight up didn't work, because the paths assumed you were cd(1)'ed into the directory.

sirupsen · 2022-05-10T14:47:06Z

docker-compose.yml

        --default-authentication-plugin=mysql_native_password
-        --innodb-buffer-pool-size=8G
-        --innodb_io_capacity=2000
-        --innodb_log_file_size=1G


Unless you have a huge computer these settings will make things way worse. MySQL OOM'ed on my machine (errors I got this morning) because the buffer pool size is bigger than I allow Docker. We have to err towards robustness over performance here. These settings are too idiosyncratic

sirupsen · 2022-05-10T14:47:24Z

docker-compose.yml

        - '5432'
      env_file:
-        - dev.env
+        - dev/dev.env


You should run docker-compose at the top-level directory, not have to cd around to develop. Too annoying.

sirupsen · 2022-05-10T14:50:08Z

pyproject.toml


 [tool.poetry.dev-dependencies]
-protobuf = "^3.20.1"
+mysql-connector-python = "*"


It's too easy to miss to install the extras otherwise for development. It's annoying to have to repeat them. If someone knows how to make them mandatory in dev, but optional in prod, let me know!

However, this will create errors. Since you are not also enforcing the installation of dependencies! You cannot simply install these packages without the correct dependencies installed. This is why we also had this docker setup. So people aren't forces to install dependencies in order to use it.

There is a reason that the docker installs python3-dev libpq-dev python3-setuptools gcc. If your system does not have those, you can't install data-diff with mysql-connector-python or psycopg2 for example.

To be clear, I see the Docker container as a production environment or a quick way to test locally—not a development environment (I should add something to the docs about the Docker container)

I do not think the Docker setup can replace this as a development environment (I'm not sure if you are implying this, but for completeness), because developing inside of Docker is a miserable experience in my experience. Having to rebuild container at times, not having the CLI tools from your laptop, etc. That's why I do not think it should be in docker-compose at all, since that's what it implies.

As for the system libraries, that's a really good point. I see 2 options:

(1) Continue to install these as dev dependencies, but as part of the doc installs, ensure that people run the correct brew install
(2) Revert back to extras, but with (1)

I am defaulting to (1) because to run the test suite, you will need all the bindings soon. Furthermore, not every database uses native libraries, as many are converging on protobuf, mysql protocol, or the postgres protocol—so I don't see it as a never-ending nightmare. If it spins out of control, I'd then suggest we move to (2)

What do you think?

We're talking about someone who wants to develop this tool, and use our env setup for it, but doesn't want to install mysql? I'm not seeing the point here.

For using the tool, yes, we want to make it easy to install. But for development, I think it's okay to raise the bar a little bit. The tests won't run without mysql anyway.

@erezsh so to be clear, you're voting for (1)? People will run mysql through Docker, so they won't have the bindings by default—so we will just provide them the instructions for it

You're on Windows though, right?

OK, let's try (1). But then we need to test it rigorously with colleagues it they can get it up and running, solely based on the doc, before we ask customers to do it.

👍🏻 I'll fold those instructions into the README and get this merged

Yes, I'm voting 1. Asking to install libraries is common for public dev setups.

Maybe we can provide a nice script for them that does it for ubuntu/mac

Yes, I use windows, but I run data-diff on WSL

sirupsen · 2022-05-10T14:50:26Z

tests/test_diff_tables.py

+        self.connection.query("DROP TABLE IF EXISTS ratings_test", None)
+        self.connection.query("DROP TABLE IF EXISTS ratings_test2", None)
+        self.preql.load("./tests/setup.pql")
+        self.preql.commit()


Make sure tables are created before creating the segment class

sirupsen · 2022-05-10T14:52:50Z

Dockerfile

+# For now while we are in heavy development we install the latest with Poetry 
+# and execute directly with Poetry. Later, we'll move to the released Pip package.
+RUN poetry install -E preql -E mysql -E pgsql -E snowflake
+ENTRYPOINT ["poetry", "run", "python3", "-m", "data_diff"]


This avoids having to type out all the arguments

So this would work and ENTRYPOINT "poetry run python3 -m data_diff" wouldn't?

They'd both work. Just been bit many times by string syntax from quoting, so in this habit now. It's what they recommend too.

Did you test this so that it works? How are you providing it with the arguments like DB1_URI etc?

sirupsen · 2022-05-10T14:53:06Z

Dockerfile

+WORKDIR /app
+# For now while we are in heavy development we install the latest with Poetry 
+# and execute directly with Poetry. Later, we'll move to the released Pip package.
+RUN poetry install -E preql -E mysql -E pgsql -E snowflake


Probably can remove extras since they should be done by default in the dev env

sirupsen · 2022-05-10T14:53:37Z

tests/test_diff_tables.py

+
        self.table = TableSegment(TestDiffTables.connection,
-                                  (self.table_name, ),
+                                  ('ratings_test', ),


The test tables were renamed without the tests being fixed

erezsh

Looks okay

erezsh · 2022-05-10T15:32:26Z

README.md

-Use cases:
+- Validate that a table was copied properly
+- Be alerted before your customer finds out, or your report is wrong
+- Validate that your replication mechnism is working correctly


Spelling error (mechnism)

cfernhout

Also check the comment from Erez with spelling mistake.

cfernhout · 2022-05-10T16:33:03Z

Dockerfile

+# For now while we are in heavy development we install the latest with Poetry 
+# and execute directly with Poetry. Later, we'll move to the released Pip package.
+RUN poetry install -E preql -E mysql -E pgsql -E snowflake
+ENTRYPOINT ["poetry", "run", "python3", "-m", "data_diff"]


So this would work and ENTRYPOINT "poetry run python3 -m data_diff" wouldn't?

cfernhout · 2022-05-10T16:42:59Z

docker-compose.yml

    mysql:
      container_name: mysql
      image: mysql:oracle
+      # fsync less aggressively for insertion perf for test setup


It's unclear where this commend is pointing to.

Basically all the configuration you put in is this, but I will refer to it 👍🏻

cfernhout · 2022-05-10T16:46:50Z

pyproject.toml


 [tool.poetry.dev-dependencies]
-protobuf = "^3.20.1"
+mysql-connector-python = "*"


However, this will create errors. Since you are not also enforcing the installation of dependencies! You cannot simply install these packages without the correct dependencies installed. This is why we also had this docker setup. So people aren't forces to install dependencies in order to use it.

cfernhout · 2022-05-10T16:50:31Z

pyproject.toml


 [tool.poetry.dev-dependencies]
-protobuf = "^3.20.1"
+mysql-connector-python = "*"


There is a reason that the docker installs python3-dev libpq-dev python3-setuptools gcc. If your system does not have those, you can't install data-diff with mysql-connector-python or psycopg2 for example.

cfernhout · 2022-05-10T16:53:43Z

README.md

+preql -f dev/prepare_db.pql mysql://mysql:Password1@127.0.0.1:3306/mysql
+preql -f dev/prepare_db.psq snowflake://<uri>
+preql -f dev/prepare_db.psq mssql://<uri>
+preql -f dev/prepare_db_bigquery.pql bigquery:///<project> # Bigquery has its own


Suggested change

preql -f dev/prepare_db_bigquery.pql bigquery:///<project> # Bigquery has its own

preql -f dev/prepare_db_bigquery.pql bigquery:///<project> # Bigquery has its own script

cfernhout · 2022-05-10T16:57:54Z

README.md


 It's highly recommended that all involved columns are indexed.

+## Development Setup


Are you sure that we only want to have doc-based instructions for a setup? Check DAT-3098

postgres support 3 part id

sirupsen commented May 10, 2022

View reviewed changes

sirupsen force-pushed the dev-env-fixes branch from 6d33dbc to 718f9b6 Compare May 10, 2022 14:48

dev: simplify

ac59a62

sirupsen force-pushed the dev-env-fixes branch from 718f9b6 to ac59a62 Compare May 10, 2022 14:49

sirupsen commented May 10, 2022

View reviewed changes

sirupsen requested review from cfernhout and erezsh May 10, 2022 14:50

sirupsen commented May 10, 2022

View reviewed changes

erezsh approved these changes May 10, 2022

View reviewed changes

cfernhout suggested changes May 10, 2022

View reviewed changes

readme: add deps

4dfdf4f

sirupsen merged commit 0097e06 into master May 11, 2022

sirupsen deleted the dev-env-fixes branch May 11, 2022 14:52

nolar pushed a commit that referenced this pull request Apr 6, 2023

Merge pull request #18 from datafold/postgres_3_part_id

40159a3

postgres support 3 part id


		It's highly recommended that all involved columns are indexed.

		## Development Setup

	preql -f dev/prepare_db_bigquery.pql bigquery:///<project> # Bigquery has its own
	preql -f dev/prepare_db_bigquery.pql bigquery:///<project> # Bigquery has its own script

dev: simplify #18

dev: simplify #18

Uh oh!

Conversation

sirupsen commented May 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sirupsen May 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erezsh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cfernhout left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

sirupsen commented May 10, 2022 •

edited

Loading

sirupsen May 10, 2022 •

edited

Loading