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

GeoTrellis 3.2.0 #112

Merged
merged 6 commits into from
Dec 18, 2019
Merged

GeoTrellis 3.2.0 #112

merged 6 commits into from
Dec 18, 2019

Conversation

echeipesh
Copy link
Collaborator

@echeipesh echeipesh commented Dec 18, 2019

Overview

  • Cross scala version dependencies diverge (ref: Switch to scala 2.12 as build default locationtech/geotrellis#3132)

  • Minimize dependencies for mamlJVM

    • logging dependency becomes Test
    • spark-sql dependency becomes Test
    • lose geotrellis-spark dependency
    • lose geotrellis-s3 dependency
  • switch from scala-logging to log4s

  • ParallelInterpreter forked for scala 2.11 and 2.12 based on changes in cats.Parallel interface

Checklist

  • Description of PR is in an appropriate section of the CHANGELOG and grouped with similar changes if possible

- Cross version dependencies diverge
- Minimize dependencies
- switch to log4s
Copy link

@notthatbreezy notthatbreezy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - might want to upgrade scala 12 version to 2.12.10 while you're here

}
}

def spark(module: String) = Def.setting {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why split spark and geotrellis out if the version doesn't change based on scala version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frivolous honestly, mostly trying to minimize the amount of lines that can duplicate version information but not sure this that much better.

@echeipesh
Copy link
Collaborator Author

[error] /home/circleci/project/jvm/src/main/scala/eval/ParallelInterpreter.scala:16:19: wrong number of type arguments for cats.Parallel, should be 1
[error]     implicit Par: Parallel[F, G],
[error]                   ^
[error] /home/circleci/project/jvm/src/main/scala/eval/ParallelInterpreter.scala:33:27: could not find implicit value for parameter P: cats.Parallel[F]
[error]       expression.children parTraverse { expr =>
[error]                           ^
[error] /home/circleci/project/jvm/src/main/scala/eval/ParallelInterpreter.scala:60:19: wrong number of type arguments for cats.Parallel, should be 1
[error]       implicit P: Parallel[T, U],
[error]                   ^
[error] three errors found

This is one of the API breaks sadly

Cats 1.6
https://github.com/typelevel/cats/blob/2fa0956a1a0b560bf12fb18a2b85c935e5aab342/core/src/main/scala/cats/Parallel.scala#L57

Cats 2.0
https://github.com/typelevel/cats/blob/c8244d433f8f36183077e4480283b1333c0f55fb/core/src/main/scala/cats/Parallel.scala#L58

Copy link
Member

@pomadchin pomadchin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can minimize that compact package to be a single type alias.

Scala 2.11

package com.azavea.maml

package object compact {
  type ParallelCompact[M[_], F[_]] = cats.Parallel[M, F]
}

Scala 2.12

package object compact {
  type ParallelCompact[M[_], F[_]] = cats.Parallel.Aux[M, F]
}

Yes the signature would still remain a legacy one, but we won't have to support two aobsolutely different Interpreter versions with different InterpreterSpecs.

I also will prepare a PR, so feel free to decline my review if you don't like it.

Copy link
Member

@pomadchin pomadchin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah also a minor thing probably not related to this PR; but is there a plan to fix packages structure in this project?

@pomadchin pomadchin self-requested a review December 18, 2019 17:26
@echeipesh echeipesh merged commit 0baee67 into geotrellis:develop Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants