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

Enabling PostgreSQL support #2855

Closed

Conversation

AlexCuadron
Copy link
Contributor

@AlexCuadron AlexCuadron commented Dec 11, 2023

This PR addresses issues #177 and #1138 by enabling SkyPilot to support PostgreSQL in addition to SQLite. While it introduces PostgreSQL as the default database, the implementation ensures backward compatibility with SQLite. If a PostgreSQL connection cannot be established, SkyPilot will automatically revert to using SQLite. This dual-database approach allows for versatile usage: SQLite can be utilized within deployed VMs, while PostgreSQL can be employed in the environment where the Sky CLI operates.

We have duplicated and adapted all smoke tests to verify SkyPilot's compatibility with PostgreSQL. This process is carried out in test_postgresql.py, wherein a PostgreSQL instance is launched within a Docker container specifically for testing. The tests then connect to this PostgreSQL instance and operate it. These tests are meticulously designed to yield output identical to their SQLite equivalents. After each test is completed, the PostgreSQL Docker instance is terminated, ensuring a clean environment for the subsequent test.

Additionally, we have executed all standard smoke tests to ensure comprehensive coverage.

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Any manual or new tests for this PR (please specify below)
    All the new tests inside test_postgresql.py
  • All smoke tests: pytest tests/test_smoke.py
  • Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
  • Backward compatibility tests: bash tests/backward_comaptibility_tests.sh

@AlexCuadron AlexCuadron marked this pull request as draft December 11, 2023 14:57
@AlexCuadron AlexCuadron marked this pull request as ready for review December 11, 2023 15:40
@Michaelvll Michaelvll self-requested a review December 15, 2023 03:06
Copy link
Contributor

This PR is stale because it has been open 120 days with no activity. Remove stale label or comment or this will be closed in 10 days.

Copy link
Contributor

This PR is stale because it has been open 120 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Sep 22, 2024
Copy link
Contributor

github-actions bot commented Oct 2, 2024

This PR was closed because it has been stalled for 10 days with no activity.

@github-actions github-actions bot closed this Oct 2, 2024
@gufranmirza
Copy link

gufranmirza commented Oct 15, 2024

@Michaelvll @romilbhardwaj, I was wondering if you are still interested in adding support for Postgres. I can work on this PR to add it.

@Michaelvll
Copy link
Collaborator

Hi @gufranmirza, thanks for your interest! Yes, we still want to support a different more scalable db backend than sqlite, but we would prefer to have it more general, ie it can be supported with different db backend. One proposal is to use sqlalchemy. Wdyt?

@gufranmirza
Copy link

@Michaelvll - It makes sense to use SQLAlchemy as it can support different database backends.

@AlexCuadron
Copy link
Contributor Author

@gufranmirza Thanks for reactivating the discussion on this PR. @Michaelvll think using SQLAlquemy makes sense. I'll refactor the PR to use it

@Michaelvll
Copy link
Collaborator

Thanks @AlexCuadron for picking this up! As the first step, using sqlalchemy to support the backend of sqlite and postgreSQL (AWS RDS) should be good enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants