Skip to content
This repository was archived by the owner on May 17, 2024. It is now read-only.

Embed sqeleton with git history preserved #485

Merged
merged 116 commits into from
Apr 20, 2023
Merged

Embed sqeleton with git history preserved #485

merged 116 commits into from
Apr 20, 2023

Conversation

nolar
Copy link
Contributor

@nolar nolar commented Apr 6, 2023

We modify & release sqeleton & data-diff in sync quite often. The latter strictly depends on newer versions of the former. However, the former (sqeleton) is not widely adapted as a standalone library, so this process is rather a burden and does not bring benefits. Also see #480 (comment)

It would be easier to modify the source code & release the data-diff library in one step. So, we merged data-diff & sqeleton for now. But we keep the possibility of their splitting indefinitely later.

sqeleton remains a top-level package for backward compatibility with the existing code (e.g. import sqeleton).

sqeleton is also moved deep into the data_diff package, so that it does not conflict with the existent package name if it is preinstalled in the virtualenvs from before.

The git history & code authorship are preserved. Sqeleton's git tags are lost — not to cause conflict with data-diff's tags. However, the historic version numbers are available in the code. Sqeleton's metadata is anyway removed after the merge.

👀Reviewers: There are too many commits. Only look at the latest two where the merge happens — named "Embed sqeleton into data-diff for synchronous changes & releases".

TODOs:

  • ❓ How do we uninstall sqeleton from the existing venvs and pinned requirements — to avoid the package name conflict?
    • (The fallback plan: rename it or move it to data_diff.sqeleton. The only drawback is a lengthy package name.)

Essentially, this was done:

git remote add sqeleton [email protected]:datafold/sqeleton.git
git fetch sqeleton

git checkout -b embed-sqeleton
git merge --allow-unrelated-histories sqeleton/master 

Then resolve all the conflicts, shuffle the files in their appropriate places (CI, docs, package metadata), cross-link then as needed, commit and push.


dbt-core upgrade and/or TUI+textual removal: It is caused by a version conflict in data-diff - sqeleton - textual - typing-extension chain. It can be revealed in a pure-sqeleton venv:

$ poetry add dbt-core==1.0.9

Updating dependencies
Resolving dependencies... (1.0s)

Because dbt-core (1.0.9) depends on typing-extensions (>=3.7.4,<3.11)
 and textual (0.9.1) depends on typing-extensions (>=4.0.0,<5.0.0), dbt-core (1.0.9) is incompatible with textual (0.9.1).
And because no versions of textual match >0.9.1,<0.10.0, dbt-core (1.0.9) is incompatible with textual (>=0.9.1,<0.10.0).
So, because sqeleton depends on both textual (^0.9.1) and dbt-core (1.0.9), version solving failed.

Version 1.0.0 is the minimum allowed in data-diff now. The conflicting requirement (link):

        'typing-extensions>=3.7.4,<3.11',

It was relaxed in dbt-core 1.1.0 — the next one after 1.0.9 (link). The latter also requires Python>=3.7.2, which is not a big deal to upgrade.

It was not noticed because the sqeleton[tui] was not installed in CI, but the issue was always there. To resolve it, we either have to (1) upgrade dbt-core from ^1.0.0 to ^1.1.0, but this will break things for dbt-core 1.0.x users, if any; (2) drop sqeleton’s “connection editor” in the embedded code; (3) make that TUI runnable only if textual is installed, fail otherwise, but do not put it in the dependencies explicitly — to be decided.

@nolar nolar mentioned this pull request Apr 6, 2023
@wvanbergen wvanbergen requested a review from vvkh April 14, 2023 14:28
@wvanbergen
Copy link

Needs some conflict resolution, but let's get this done ASAP.

@nolar nolar marked this pull request as ready for review April 20, 2023 11:44
Otherwise, it will conflict with the existing standalone package of that name, unless it is manually remove from virtualenvs.
@nolar
Copy link
Contributor Author

nolar commented Apr 20, 2023

This force-push is a replay of the same changes on top of the newer main branches in both repos.

Also, sqeleton docs are removed. The TUI for "connection editor" is also removed to resolve the package conflict for Textual (see the PR description).

@nolar nolar merged commit 50bac04 into master Apr 20, 2023
@nolar nolar deleted the embed-sqeleton branch April 20, 2023 12:08
@wvanbergen
Copy link

Thanks @vvkh and @nolar for making this happen.

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

Successfully merging this pull request may close these issues.

10 participants