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

Update r5r to use latest version of JDK v20 #350

Closed
rafapereirabr opened this issue Sep 13, 2023 · 13 comments
Closed

Update r5r to use latest version of JDK v20 #350

rafapereirabr opened this issue Sep 13, 2023 · 13 comments
Assignees
Labels

Comments

@rafapereirabr
Copy link
Member

Available from either:

We are using JDK v20 over at r5py , so it should be possible to use it in r5r.

@ttuff
Copy link

ttuff commented Sep 18, 2023

Can you describe a little further? I've tried this a few times and I still throw an error in setup_r5() asking specifically for version 11. My script previously worked but now it doesn't.

rJava::.jinit()
[1] 0
rJava::.jcall("java.lang.System", "S", "getProperty", "java.version")
[1] "20.0.2"

r5r_core <- setup_r5("r5r", verbose = FALSE)
Error in setup_r5("r5r", :
This package requires the Java SE Development Kit 11.
Please update your Java installation. The jdk 11 can be downloaded from either:

@ttuff
Copy link

ttuff commented Sep 18, 2023

also..
java -version
openjdk version "20.0.2" 2023-07-18
OpenJDK Runtime Environment (build 20.0.2+9-78)
OpenJDK 64-Bit Server VM (build 20.0.2+9-78, mixed mode, sharing)

@rafapereirabr
Copy link
Member Author

Hi @ttuff . The curent version of r5r only supports JDK 11. So the package will throw an error if R does not detect JDK 11 installed locally.

However, the new version of R5 can be used with together JDK 20. I've opened this issue because we're planning to support JDK 20 in the next versio of r5r. However, this has not been implemented in our R package yet. When we complete this taks, we will close this issue.

@ttuff
Copy link

ttuff commented Sep 18, 2023

Thanks for all your hard work on this. If I use the python version, it will work with JDK 20?

@rafapereirabr
Copy link
Member Author

yes it will !

@higgicd
Copy link

higgicd commented Oct 11, 2023

Here's hoping this also solves the fatal errors mentioned in #315 with the availability of newer native java builds for M-series Macs (without having to resort to the work arounds mentioned in that thread)

@rafapereirabr
Copy link
Member Author

I hope so too! @mvpsaraiva will be having a look at this issue in the next couple of days. Fingers crossed Marcus wil bring good news!

@mvpsaraiva
Copy link
Collaborator

Hi, @rafapereirabr and @ttuff. I've been looking into this and I have some partially good news.

We are currently stuck with JDK 11 because R5 uses the Kryo library to serialize the network.dat, so the road+transit network can be cached and reused later. I think r5py avoids this issue because they don't use R5's network serializer, they rather rebuild the network every time. That's what I understood from the issue 367.

R5 officially still uses JDK 11 LTS, and Andrew is currently working on a PR to upgrade it to JDK 21 LTS (PR 760). Once that's done, we can upgrade as well.

So, I think our two options are:

  1. Check if the JDK version is > 11 and, if true, do not use the serializer and rebuild the network every time. I've tried this, and it mostly works (detailed_itineraries is broken, though... I need to investigate further). For large networks, this can add a significant amount of processing time.
  2. Wait a little longer for R5 to officially upgrade to JDK 21. That PR is almost complete, so it may not take much time.

I'll try option 1 in the next few days. If I can fix detailed_itineraries, I'll implement this upgrade and then we can move to option 2 once R5 is also updated.

@rafapereirabr
Copy link
Member Author

This is really great news, @mvpsaraiva . It looks like Conveyal will be merging Andrew's PR soon. In this case, it sounds like perhaps we could wait for the next release of R5. What do you think ?

@mvpsaraiva
Copy link
Collaborator

This is really great news, @mvpsaraiva . It looks like Conveyal will be merging Andrew's PR soon. In this case, it sounds like perhaps we could wait for the next release of R5. What do you think ?

Indeed, I think R5's update is coming soon, so it's better we wait a little more.

@abyrd
Copy link

abyrd commented Nov 29, 2023

Hi everyone! We thought you might be interested in this: https://github.com/conveyal/r5/releases/tag/v7.0

@rafapereirabr
Copy link
Member Author

Thanks so much for the update, @abyrd ! We are currently working towards an update of r5r to use the version 7 of R5 . @mvpsaraiva is looking at this at the moment

@rafapereirabr
Copy link
Member Author

The dev version of r5r is now working with the latest V7 of R5 and JDK 21, thanks to PR #359 and a few subsequent commits. Thanks @mvpsaraiva ! Closing this issue for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants