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

Scala 2.12 branch #37

Open
stuhood opened this issue Feb 21, 2017 · 33 comments
Open

Scala 2.12 branch #37

stuhood opened this issue Feb 21, 2017 · 33 comments

Comments

@stuhood
Copy link

stuhood commented Feb 21, 2017

Twitter consumes some of the twofishes libraries, and are interested in helping to get a 2.12 branch started. This is a notice of our intent to do so!

@mateor
Copy link
Contributor

mateor commented Feb 21, 2017

👋

Yeah...one of the drawbacks of the Fsq.io mode is that it can chain users to our internal versions. It most often is a problem for our Pants modules, actually.

The state of fsqio master is directly split off of our monorepo so in order to convert Fsqio/master we would have to upgrade our internal codebase. Seeing as that is not imminent, I am okay with running a publish from a 2.12 branch of Fsq.io.

One open question is that the io.fsq publishing model is to publish all jars as a unit - we are essentially versioning the repo itself (most recently 1.32). We could be willing to compromise on that as needed but have a strong preference of maintaining that practice.

I am going to ping a few coworkers with domain knowledge (You already spoke with @jvandew and @tdyas is Tom Dyas who tagged along to the last Pants summit).

@jvandew
Copy link
Contributor

jvandew commented Feb 22, 2017

Twofishes at least will have issues with the Salat library it uses. There are apparently plans for a 2.12 release branch there, but no real timeline: salat/salat#183. We do already have a pull request open with them that might address the issues raised in that issue though, so possibly we could just build and host a 2.12 jar of that patch in the meantime.

Long term I would really like to see us rewrite the mongo layer of the indexer to use Rogue. It's pretty disparate from the rest of our stack as is and I think #33 has shown that the current state is not maintainable going forward. I don't think that would be too tricky to make happen, just a matter of someone finding the time for it.

@stuhood
Copy link
Author

stuhood commented Apr 30, 2017

The first things I encountered while attempting the upgrade:

  • com.github.salat#salat and org.clapper#argot do not appear to be published for 2.12... the latter looks bitrotted

(unrelated: it seems like the upkeep script might have a dependency on binary builds from somewhere, as it fails to create a virtualenv ... I was able to get past that by replacing the pants script in the repo with the one from www.pantsbuild.org/install.html)

@mateor
Copy link
Contributor

mateor commented May 3, 2017

Hey Stu.

Let's see - so I think we have a solution for salat- I have been holding off pushing an fsqio update because of some internal churn. But we have inlined the one class from Salat we use, I can push that out soon (better would have been to consume the shading rules, but we didn't get there).

The clapper jar looks like something we can rip out. It is just an arg parser used by spindle - any more modern arg parser would do just as well.

I would be interested in seeing the upkeep failure - the Travis build passed and I know we have people using this repo. But I could believe that there is a lingering failure state still in there somewhere.

@mateor
Copy link
Contributor

mateor commented May 16, 2017

@tdyas and @jvandew have closed these issues internally. I will update fsqio sometime before next Monday.

@stuhood
Copy link
Author

stuhood commented May 18, 2017

Huzzah! Sorry that I was so useless here.

@mateor
Copy link
Contributor

mateor commented May 30, 2017

So, I was wrong about this being done - I apologize Stu : (

We had inlined a class from Salat in order to upgrade our driver - I thought that meant we were finished with the dependency. But that was inaccurate - there are plenty of Salat callsites in twofishes itself.

Salat is a serialization library and this is the only project in our repo that uses it. Internally, our solution would be to port it to Rogue. I am considering how we could make that happen, but it is obviously a bigger job than just pushing out an Fsq.io update.

In the meantime, I do not have a good answer for you here. We could consider just trying to publish a shaded salat, or expand our inlined fork.

@landism
Copy link

landism commented Aug 4, 2017

salat 1.11.1 has been published for 2.12: http://search.maven.org/#search%7Cga%7C1%7Csalat

@mateor
Copy link
Contributor

mateor commented Aug 18, 2017

Wow, thanks. Jacob just linked me to this, I missed that comment! We can probably get move to that for now, then. Let me talk with some folks and I will follow up.

@stuhood
Copy link
Author

stuhood commented Oct 13, 2017

How's this going?

@tdyas
Copy link
Contributor

tdyas commented Oct 17, 2017

cc @jvandew and @mateor

@mateor
Copy link
Contributor

mateor commented Oct 27, 2017

Hi - we asked @wcauchois to take this up.

@wcauchois
Copy link
Contributor

Hey, just to update on this, I ended up ripping out Salat and porting the twofishes index builder code to use Rogue (kind of the nuclear option, but also ended up making some code nicer :). That PR is merged internally and we should be able to publish it soon!

@stuhood
Copy link
Author

stuhood commented Nov 13, 2017

Awesome... thanks @wcauchois!

@mateor
Copy link
Contributor

mateor commented Dec 18, 2017

Hi - this had the unlucky timing to land in the middle of our converting Fsq.io (and our internal repo) from pom-resolve to Ivy.

But that update has been shipped. The 2.0.2 jars for twofishes have been published for Scala 2.11. We have internal blockers to 2.12 still, but the jar publish is now using standard Pants mechanisms and you should be able to interact with them using common methods

@mateor
Copy link
Contributor

mateor commented Dec 18, 2017

Current 2.11 jars are 2.02 and above

@stuhood
Copy link
Author

stuhood commented Mar 13, 2018

Hey Mateo! Have you had any luck with your migration?

@mateor
Copy link
Contributor

mateor commented Mar 15, 2018

The last requirement for upgrading Fsq.io to Scala 2.12 is Finagle upgrade, but that is blocked internally by our web team. They are doing that work this quarter but I can't promise when that will happen.

But I did knock over the 2.12 blockers for fsq.io as you identified them back in the original issue. The Twofishes and Spindle library has no blockers to Scala 2.12. As a bonus, the resolve and publish mechanisms have been ported off pom-resolve and should be ~roughly familiar to upstream Pants.

@mateor
Copy link
Contributor

mateor commented Mar 15, 2018

I can try and configure a ~working resolve and post what the migration for twofishes alone might look like

@stuhood
Copy link
Author

stuhood commented Mar 16, 2018

@mateor : Thanks Mateo! If those issues are resolved and the repo is on a more recent pants version with fewer plugins, then I think I might be unblocked already. Will take a look soon.

@mateor
Copy link
Contributor

mateor commented Apr 18, 2018

Yeah, as a part of this project I deprecated pom-resolve and pom-publish, so this was a slow burn for sure.

We did make a 2.12 branch and discovered two additional issues. One was our own foursquare.country-revgeo jar, which was being published by david blackman from a separate repo, presumably on your behalf :)

I moved that jar into Fsq.io, bootstrapping the shapefiles at runtime: 646692d

The other issue is another dependency that is not published for 2.12, jerkson. We have assigned @iantabolt to that issue and he is attacking it this sprint.

@mateor
Copy link
Contributor

mateor commented Apr 18, 2018

I believe stu is on vacation now, lets try and close this issue in April

@mosesn
Copy link

mosesn commented May 14, 2018

@mateor what's the current status on this? I work with @stuhood

@mateor
Copy link
Contributor

mateor commented May 14, 2018

Hi @mosesn. We made progress, by virtue of small cuts. I think you are probably familiar with our core issue, which is Finagle upgrade, specifically in our internal web framework. Migrating over to httpx and back has been a war of attrition.

I actually spent most of today preparing an Fsq.io update that removes jerkson (thanks @iantabolt ), which was the current blocking issue. That update should be mostly ready to go. When it comes out I will check the 2.12 branch I have stashed and see what we see.

@mosesn
Copy link

mosesn commented May 15, 2018

OK, I'm happy to answer any questions you have about finagle. I also moved back to NYC, so I could swing by and chat some day if you want.

@mateor
Copy link
Contributor

mateor commented May 15, 2018

Hey welcome back - I just may try and take you up on that :)

I was able to update Fsq.io last night - the big contribution towards this issue was by Ian Tabolt , removing jerkson.

I have a dummy fork of 2.12 fsq.io I test resolution with as we make progress. I think we are down to one last dependency we need to upgrade/remove. Here is my commit message from that branch:

    Updated resolution for Scala 2.12

    Only one left - for the resolve, that is. We will still
    need to debug whatever compile issues we find afterwards

    ```
                                    ::::::::::::::::::::::::::::::::::::::::::::::

                                    ::          UNRESOLVED DEPENDENCIES         ::

                                    ::::::::::::::::::::::::::::::::::::::::::::::

                                    :: com.rockymadden.stringmetric#stringmetric-core_2.12;0.27.4: not found

                                    ::::::::::::::::::::::::::::::::::::::::::::::

    ```
    Surprise surprise, it is used in the geocoder

    ```
    mateo-2:fsqio mateo$ git grep rockymadden
    3rdparty/BUILD.opensource:  name = 'rockymadden',
    3rdparty/BUILD.opensource:    scala_jar(org = 'com.rockymadden.stringmetric', name = 'stringmetric-core', rev = '0.27.4')
    src/jvm/io/fsq/twofishes/indexer/importers/geonames/BUILD:    '3rdparty:rockymadden',
    src/jvm/io/fsq/twofishes/indexer/importers/geonames/PolygonLoader.scala:import com.rockymadden.stringmetric.similarity.JaroWinklerMetric
    src/jvm/io/fsq/twofishes/indexer/importers/geonames/PolygonLoader.scala:import com.rockymadden.stringmetric.transform._
    src/jvm/io/fsq/twofishes/indexer/scalding/BUILD:    '3rdparty:rockymadden',
    src/jvm/io/fsq/twofishes/indexer/scalding/BaseUnmatchedPolygonFeatureMatchingIntermediateJob.scala:import com.rockymadden.stringmetric.similarity.JaroWinklerMetric
    src/jvm/io/fsq/twofishes/indexer/scalding/BaseUnmatchedPolygonFeatureMatchingIntermediateJob.scala:import com.rockymadden.stringmetric.transform._
    src/python/fsqio/pants/buildgen/jvm/scala/third_party_map_jvm.py:    'rockymadden': 'rockymadden',
    ```

This is core geocoder code so possibly not so fun to rip out. I will again lean on @iantabolt, who was on our venues team before he transitioned to infra.

I was going to have him pick up our finagle upgrade next sprint - I will ask if he can tackle this issue as a prerequisite.

@stuhood
Copy link
Author

stuhood commented May 17, 2018

@mateor : Awesome work! Glad to see that you guys are so close.

I was able to hack around that last dep in order to do a private publish of twofishes-countryinfo, so as far as I can tell we're unblocked over here. Sorry that we were not able to do more to help with the upgrade, but very very happy to see the fsqio build working out of the box. Exciting times!

@mateor
Copy link
Contributor

mateor commented May 18, 2018

Man...I am so glad to hear that. I had self-assigned it but was scrambling to find the time.

I was glad to make progress towards Scala 2.12 but realistically that has a ways to go still. It took significant resources to convert to 2.11 in 2017 and we expect to need two independent Finagle upgrades to safely migrate our internal web stack for 2.12. But even without getting to a full 2.12 upgrade yet, I am relieved our steady efforts got you unblocked.

Did you just publish the 2.12 fork of rockymadden? I would be pretty interested in looking at your 2.12 patch, if you can share it

The various tickets this issue spawned inspired a near overhaul of our build stack, including deprecating pom-resolve, vexsrc and pom-publish in favor of upstream Pants tasks. Some public thanks to the many folks who volunteered their cycles pushing this rock up the mountain.

From: @jvandew

From: @wcauchois

From @mateor:

From: @tdyas

From: @TansyArron

From @iantabolt

@StephanSchmidt
Copy link

Sorry to be bothersome, some updates after 3months? :-)

@mateor
Copy link
Contributor

mateor commented Aug 23, 2018

Twitter said they had what they needed. What do you need?

@StephanSchmidt
Copy link

As this project is dead, could some close it please? People might accidentally use it.

@jvandew
Copy link
Contributor

jvandew commented Mar 18, 2019

This scala 2.12 issue? We are still working on our 2.12 migration internally and will get that work pushed here once we're upgraded.

If you need 2.12 artifacts published in the meantime we can likely unblock you there, but I will refer you back to mateor's question from last August: what do you need?

@StephanSchmidt
Copy link

StephanSchmidt commented Mar 18, 2019

Ok, I need to run Rogue with 2.12, sorry didn't understand the question correctly the last time.

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

No branches or pull requests

8 participants