allow dataset connection to sample, improve error handling #7
Conversation
wow-such-code
commented
Sep 2, 2024
wow-such-code
commented
- allows users to provide a sample identifier to upload datasets (previously only experiments were possible)
- improves error handling when PETab directory is not specified for upload
- removes unnecessary classes
- provides some SEEK classes and methods, that are not yet used
There was a problem hiding this comment.
Heya @wow-such-code I have some questions and minor nitpicks, but overall it looks good! Well done 👍
| return; | ||
| } | ||
| DatasetWithProperties result = new DatasetWithProperties(datasets.get(0)); | ||
| Optional<String> patientID = openbis.findPropertyInSampleHierarchy("PATIENT_DKFZ_ID", |
There was a problem hiding this comment.
Could there be other PatientIds classifiers? If so i'd recommend to move this into a global identifier.
| System.out.println("Found dataset, downloading."); | ||
| System.out.println(); | ||
|
|
||
| final String tmpPath = "tmp/"; |
There was a problem hiding this comment.
I'm confused of the purpose of this command? It downloads the data into a temp folder and afterwards deletes it again?
| System.out.printf("Dataset %s was successfully created%n", result.getPermId()); | ||
| } else { |
There was a problem hiding this comment.
NIT: I think it would help to have 2 different output messages here, since one result attaches the dataset to an experiment while the other registers it to a sample
| DataSetPermId result = openbis.registerDatasetForExperiment(Path.of(dataPath), experimentID, parents); | ||
| System.out.printf("Dataset %s was successfully created%n", result.getPermId()); |
There was a problem hiding this comment.
Do you need to differentiate between registration to an experiment and to a sample here as well?
There was a problem hiding this comment.
the use case here was specified as always using an experiment
| } catch (final Exception e) { | ||
| LOG.error(e.getMessage()); | ||
| } | ||
| return null; |
There was a problem hiding this comment.
Why does this method specify a return value if it always returns a null value?
There was a problem hiding this comment.
it returns a permID if the try case succeeds
| creation.setUploadId(uploadId); | ||
| creation.setParentIds(parentCodes.stream().map(DataSetPermId::new).collect( | ||
| Collectors.toList())); | ||
| creation.setTypeId(new EntityTypePermId("UNKNOWN", EntityKind.DATA_SET)); |
There was a problem hiding this comment.
This also looks hardcoded? Maybe having some JD or doing this case in a seperate method would help distinguish on how this can happen?
(Can there be multiple Unknown PermIds?)
There was a problem hiding this comment.
this is the openbis dataset type, it is not clear yet which types should be used and how they will be specified
src/main/java/life/qbic/io/commandline/UploadDatasetCommand.java
Outdated
Show resolved
Hide resolved
src/main/java/life/qbic/io/commandline/UploadDatasetCommand.java
Outdated
Show resolved
Hide resolved
| } catch (final Exception e) { | ||
| LOG.error(e.getMessage()); | ||
| } | ||
| return null; |
