-
Notifications
You must be signed in to change notification settings - Fork 2
upgrade delta constraint and fix ty #583
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
Conversation
Test Results (Python 3.10)1 964 tests 1 942 ✅ 6m 26s ⏱️ Results for commit 1439f72. ♻️ This comment has been updated with latest results. |
Coverage Report (Python 3.10) •
|
||||||||||||||||||||||||||||||||||||||||||||||||||
Coverage Report (Python 3.11) •
|
||||||||||||||||||||||||||||||||||||||||||||||||||
Coverage Report (Python 3.12) •
|
||||||||||||||||||||||||||||||||||||||||||||||||||
1dfeece to
1439f72
Compare
danielgafni
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple you explain the Dagster changes? They aren’t related to the title of this PR
|
yes they are related to TY; I did run the |
|
I mean, there are some non-trivial changes to code that has been working for me, with comments like "AssetDep is not hashable" - but I don't think this is true since the code has been working before? What issue is the new code addressing? I'll be able to look into it on Monday, but for now won't it be easier to remove these changes from this PR to make it obviously right? |
|
I think we do not have to rush this one; should we split it into 2x perhaps 1) for the new version of delta and 2) one for the uv upgrade which incluees and upgrade for ty? (and the hotfixes) |
|
Honestly, I'm not sure we should enforce the The deltalake issue has only affected our testing environment ( We shouldn't force this upgrade on our users since Metaxy itself doesn't require it at runtime. I've added more details in the original issue. The |
|
ok - let me close here and run the ty upgrade solo #584 |

resolves: #582