-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[dagster-embedded-elt] Expose dlt and sling extras #26497
Conversation
Deploy preview for dagster-docs-beta ready! Preview available at https://dagster-docs-beta-b7wh6wgpd-elementl.vercel.app Direct link to changed pages: |
ae1d5f3
to
586f9a8
Compare
Deploy preview for dagster-docs ready! Preview available at https://dagster-docs-63xuyaycb-elementl.vercel.app Direct link to changed pages: |
@cmpadden @PedramNavid do either of you recall why we didn't do it this way to begin with? I remember there was some discussion about having embedded-elt be its own package vs. having dagster-sling and dagster-dlt, wondering if there's some context missing there |
Can’t think of any reason why we wouldn’t do this. Will this be a breaking change? |
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.
discussed offline: this would unfortunately be a breaking change
As a user, I was about to open a issue for the same reason. Inconvenience
Potential solutionThere seems to be two-fold for backwards compatibility:
|
Hi @zilto ! Thanks for weighing in here -- we're currently planning on segmenting these libraries out essentially as you describe, although we'll likely just port the code for each integration into the new libraries and keep |
Closing this issue and creating #26523 to track the effort to split out these libraries. |
Summary & Motivation
As a user, I don't unnecessarily want to install Sling if I'm only using dlt and vice versa.
How I Tested These Changes
Changelog