Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: run pgai tests against extension from source #206

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

alejandrodnm
Copy link
Contributor

Change pgai testcontainer to build the extension and copy the extension code to a timescaledb-ha container.

This will make CI run the pgai python library against the latest changes made to the extension, and surface breaking changes sooner.

Change pgai testcontainer to build the extension and copy the extension
code to a timescaledb-ha container.

This will make CI run the pgai python library against the latest changes
made to the extension, and surface breaking changes sooner.
@alejandrodnm alejandrodnm marked this pull request as ready for review November 8, 2024 19:20
@alejandrodnm alejandrodnm requested a review from a team as a code owner November 8, 2024 19:20
Copy link
Collaborator

@jgpruitt jgpruitt left a comment

Choose a reason for hiding this comment

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

very in favor of the pgai tests using the latest extension code! 🎉

Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't work for versions 0.1.0, 0.2.0, and 0.3.0 of the extension. Maybe that doesn't matter though for the pgai library.

Comment on lines +14 to +18
COPY requirements.txt .
COPY ai/__init__.py ai/
RUN version=$(grep -oP '(?<=__version__ = ")[^"]*' ai/__init__.py) && \
mkdir -p /usr/local/lib/pgai/$version && \
pip3 install -v --no-deps --compile -t /usr/local/lib/pgai/$version -r requirements.txt
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are we doing this? make install does this already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To cache de dependencies in a Docker layer. If we change the extension code we don't need to re-install the dependencies.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah. yeah. bummer

mkdir -p /usr/local/lib/pgai/$version && \
pip3 install -v --no-deps --compile -t /usr/local/lib/pgai/$version -r requirements.txt
COPY . .
RUN make install-py build-sql install-sql
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not just make build-install??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because build_install also calls install_prior_py, and I don't need to install those deps.

ARG PG_MAJOR=16
COPY --from=build-latest /usr/share/postgresql/${PG_MAJOR}/extension/ai--*.sql /usr/share/postgresql/${PG_MAJOR}/extension/
COPY --from=build-latest /usr/share/postgresql/${PG_MAJOR}/extension/ai.control /usr/share/postgresql/${PG_MAJOR}/extension/ai.control
COPY --from=build-latest /usr/local/lib/pgai/ /usr/local/lib/pgai/
Copy link
Collaborator

Choose a reason for hiding this comment

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

for versions 0.1.0, 0.2.0, and 0.3.0, the dependencies are installed in the system path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The pgai tests are not parametrize on the extension version, so it'll only use the default version. We'd need those version if we parametrize the extension versions.

I think if we do that, instead of doing it in the code, we can do it in the ci matrix.

Copy link
Collaborator

Choose a reason for hiding this comment

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

the vectorizer code will need to be backwards compatible with prior versions of the extension, and I think we'll want tests to that end eventually, but happy to postpone

@alejandrodnm alejandrodnm merged commit ffc20d2 into main Nov 11, 2024
6 checks passed
@alejandrodnm alejandrodnm deleted the adn/worker-tests branch November 11, 2024 13:14
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.

2 participants