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

Upgrade to java 21 #272

Merged
merged 1 commit into from
Mar 22, 2024
Merged

Upgrade to java 21 #272

merged 1 commit into from
Mar 22, 2024

Conversation

mandawilson
Copy link
Collaborator

@mandawilson mandawilson commented Jan 25, 2024

Changes needed for running under java21 and for packaging this depenency into the pipelines codebase for cbioportal study import into msk databases.

@@ -32,7 +32,7 @@ jobs:

- run:
name: "compile"
command: "mvn clean install"
command: "mvn clean install -Dnet.bytebuddy.experimental=true"
Copy link
Member

Choose a reason for hiding this comment

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

what is the experimental thing for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One of the dependencies (bytebuddy from Mockito) won't work with Java 21 without it. But let me try to update Mockito and see if that fixes it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Phew! 😅 It wasn't easy but I finally got the right set of dependencies so we don't need that flag. I think the "-Dnet.bytebuddy.experimental=true" was temporarily needed for Java 21 until recently.

@mandawilson mandawilson force-pushed the upgrade-to-java21 branch 2 times, most recently from a430dc8 to efd80bb Compare January 30, 2024 17:47
@mandawilson mandawilson force-pushed the upgrade-to-java21 branch 2 times, most recently from c33927c to 079c4c2 Compare February 1, 2024 11:12
Copy link
Member

@leexgh leexgh left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! Do we need to update Java version in docker as well?

Comment on lines -80 to -94
<dependency>
<groupId>com.github.cbioportal.cbioportal</groupId>
<artifactId>maf</artifactId>
<version>ee5802d836c05ed846d7d1ea3f584febdc07ffa8</version>
<exclusions>
<exclusion>
<groupId>ch.qos.logback</groupId>
<artifactId>logback-core</artifactId>
</exclusion>
<exclusion>
<groupId>ch.qos.logback</groupId>
<artifactId>logback-classic</artifactId>
</exclusion>
</exclusions>
</dependency>
Copy link
Member

@inodb inodb Feb 13, 2024

Choose a reason for hiding this comment

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

@sheridancbio do we still want to keep this? I forgot the final verdict on this

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, for now I think we should keep this dependency. @haynescd pointed out that maf is a dependency in cbioportal-core, and since the maf code is used in both this repository and in that one we can keep it in a separate repository I think.

Hopefully we can develop a unified community-wide tool for study import which includes the ability to annotate variants (via genome nexus). That would essentially replace cbioportal-core. At that time we could decide whether the ability to parse and represent variants from a mutation file should be its own module or become embedded in one of these two repositories (most likely this one).

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that we still do have the dependency .. it is simply moved to the annotator module (it is only needed there)

@sheridancbio sheridancbio force-pushed the upgrade-to-java21 branch 4 times, most recently from 091d758 to d217c2d Compare February 14, 2024 14:08
@inodb
Copy link
Member

inodb commented Feb 22, 2024

@mandawilson @sheridancbio i noticed there's an error in the build here - are you still looking at this one? Thanks!

@sheridancbio
Copy link
Contributor

sheridancbio commented Feb 22, 2024

@mandawilson @sheridancbio i noticed there's an error in the build here - are you still looking at this one? Thanks!

Yes .. we are trying to resolve conflicts with the pipelines codebase (which also packages in cbioportal-core repository). There are spring version differences between these two codebases so we may need to make the spring version of this codebase match the spring version (and boot version) in use in cbioportal-core in order for the importer to function correctly.

So consider this PR to be in flux for now.

@sheridancbio
Copy link
Contributor

This PR seems complete now. Docker functionality has been tested. Note: the "merge" sub command mode (seen through docker build and run) is not running either in this PR or in the master branch. A new issue to repair (or delete) the merge code logic should be created (if not extant).

Studies have been annotated during import in the msk importer (which uses the annotator module as a component).

changes sets were squashed into a single changeset.

- switch to new cbioportal maf repo
- update docker to openjdk-21
- rather than upgrading databaseAnnotator, delete it due to:
	- infrequent use
	- testing challenges
	- difficult maintenance
- if databaseModule is desired by any users, an updated and tested version of the module can be re-introduced by a new PR.
- prevent JobParameters from containing null values (not allowed in spring batch 5)

Co-authored-by: Manda Wilson <[email protected]>
Co-authored-by: Robert Sheridan <[email protected]>
@sheridancbio
Copy link
Contributor

Note: the merge subcommand functionality actually did succeed when executed with the proper arguments ... basically those were: java -jar {jar_filename} merge --input-mafs-list='fileA,fileB,fileC,...' --output-maf='outfilename.txt'

I am proceeding with this merge now. A description of the potential ramifications was presented to the development team in the genomenexus slack channel in the cbioportal organization and there were no objections. This included the dropping of the databaseAnnotator module, the new requirement to build and run this codebase under a java21 jdk, and the successful testing of the annotator module as a dependency in the msk importer and also in the Docker mode execution for annotating test files.

@sheridancbio sheridancbio merged commit fcc131a into master Mar 22, 2024
1 check passed
@sheridancbio sheridancbio deleted the upgrade-to-java21 branch March 22, 2024 03:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants