Add java_opts to cbioportalImporter.py

[oplantalech]

Currently we can only pass the jar_path to cbioportalImporter.py. This adds java_opts as a new option to cbioportalImporter.py to allow passing any JAVA_OPTS parameter. This allows us pass max memory usage etc. The jar_path option is retained in a backward-compatible manner

[MatthijsPon]

Looks good, would be nice to be able to throttle memory for the loader, some questions:

[MatthijsPon]

Will this not become -cp -cp JAR_PATH OTHER_JAVA_OPTS if -cp is specified in java_opts?

[oplantalech]

No, because this line only evaluates when there is no -cp in args.java_opts

[MatthijsPon]

Why not:

if args.jar_path:
    args.java_opts = f"-cp {args.jar_path} {args.java_opts}"

Then there is no 'need' to deprecate and you can replace it with something like 'WILL BE DEPRECATED IN THE FUTURE'?

[oplantalech]

Hmm... but we want people to move away from using jar_path, right? We should then force them somehow to use the new attribute

[JREastonMarks]

I like the idea of adding java_opts as a parameter to these scripts. However, I agree with @MatthijsPon that this should be a warning at the most and we should continue to support jar_path. Asking all sites that use this to change to a new way of launching this is a big ask especially with no warning. We are doing that enough with spinning out CORE into it's own repo.

[inodb]

Since JAVA_OPTS is an env parameter, another way of going about it is to pass the environment from the main process:

https://stackoverflow.com/questions/2231227/python-subprocess-popen-with-a-modified-environment

Avoids adding additional code to handle another cli parameter and u could e.g. set java opts directly as env on the container. Not sure if that is necessary more clear to a user tho (as you're sorta passing a JAVA_OPTS env variable to a Pyhon wrapper script)

[oplantalech]

@inodb I've tried to set the JAVA_OPTS from the environment as the default for --java_opts (so only if --java_opts is not specified). Not sure if that's what we want, though. Also, if now the JAVA_OPTS from the environment defines the classpath, then we use that classpath instead of locating the JAR file relative to the import script. Is that what we want?

[inodb]

LGTM - thank you!