-
Notifications
You must be signed in to change notification settings - Fork 56
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
Bazel builds enabled along with sbt #893
base: main
Are you sure you want to change the base?
Conversation
def _safe_name(coord): | ||
return coord.replace(":", "_").replace(".", "_").replace("-", "_") | ||
|
||
def maven_artifact(coord, repository_name = "maven"): |
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.
is there anything here that could be substituted with an existing bazel integration? eg.. https://github.com/bazel-contrib/rules_jvm_external?tab=readme-ov-file
import org.apache.spark.sql.types.StructType | ||
import org.apache.spark.sql.{Encoder, Row} | ||
|
||
object EncoderUtil { |
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.
ooc what's the reason behind this class?
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.
LGTM! This is massively impactful Krish! Thanks for putting this up.
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.
Thanks for taking the time to make this PR. Swapping over to bazel would be great for chronon!
Couple of comments / qs:
- Lets also add some notes on how devs can use build / add deps etc using bazel in the devnotes
- While we're keeping sbt & bazel around in the transitionary period, is there anything we need to call out for folks to take care of while updating deps / such?
## Disable remote cache completely when --config=local is passed | ||
build:local --remote_cache= | ||
|
||
# Scala version config flags: |
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.
wdyt about moving this to devnotes?
@@ -144,6 +157,18 @@ val VersionMatrix: Map[String, VersionDependency] = Map( | |||
Some(spark3_1_1), | |||
Some(spark3_2_1) | |||
), | |||
"spark-all-3-5" -> VersionDependency( |
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.
is this spark 3.5 stuff needed for the bazel changes? If not, I'd propose splitting out to a different PR
) | ||
|
||
scala_test_suite( | ||
name = "test", |
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.
in a follow up it would be nice to split this into a test invocation we test suite rather than triggering all at once. Bazel has better rails for this iiuc compared to sbt and breaking this out would help CI a fair bit
Summary
Why / Goal
Goal is to improve the build environment(by sandboxing) and provide multiple spark versions
Test Plan
Unit testing green
quick start steps green https://chronon.ai/getting_started/Tutorial.html
Build commands
bazel build --config scala_2.12 --define spark_version=3.2 //spark/...
Checklist
Reviewers
@nikhilsimha @piyush-zlai