TASK-6442 - Add Resources Management functionality #2535

pfurio · 2024-12-10T11:37:09Z

No description provided.

…ASK-7047, #TASK-6442

…SK-6442

…alyisis resources for the current OpenCGA version, and generate clients, #TASK-7088, #TASK-6442

…o the installation folder, #TASK-7088, #TASK-6442

…SK-6442

… #TASK-6442

…lization, and add the flag fetchResourceOnInit in the configuration file, #TASK-7088, #TASK-6442

…K-6442

… #TASK-7088, #TASK-6442

…8, #TASK-6442

…SK-6442

…-7088, #TASK-6442

…SK-6442

…, #TASK-6442

…SK-6442

…ing, #TASK-7125, #TASK-6442

…esource management, #TASK-7127, #TASK-6442

…me some constants, #TASK-7088, #TASK-6442

…ng the ResourceManager, #TASK-7139, #TASK-6442

…changes, #TASK-7139, #TASK-6442

…K-7125, #TASK-6442

…hanges, #TASK-7127, #TASK-6442

…anges, #TASK-7088, #TASK-6442

…K-6442

…SK-6442

… #TASK-6442

…6442

…-7088, #TASK-6442

…, #TASK-6442

…SK-6442

…6442

…tion file according, #TASK-7125, #TASK-6442

j-coll

Review WIP

j-coll · 2025-01-21T15:57:13Z

opencga-core/src/main/java/org/opencb/opencga/core/config/Analysis.java

@@ -60,12 +72,12 @@ public Analysis setScratchDir(String scratchDir) {
        return this;
    }

-    public String getResourceUrl() {
-        return resourceUrl;
+    public Resource getResource() {


Do not replace old parameters like this. Instead, keep the old methods as "deprecated" and add a call to "Configuration#reportUnusedField"

j-coll · 2025-01-21T15:59:50Z

opencga-analysis/src/main/java/org/opencb/opencga/analysis/ConfigurationUtils.java

+
+    public static Path getToolResourcePath(String resourceId, Configuration configuration) throws ToolException {
+        // Get resources from the configuration file
+        for (ResourceFile resourceFile : configuration.getAnalysis().getResource().getFiles()) {


configuration analysis.resource is a new field in the configuration file. It might be null when upgrading from old versions.
Either add a non-null check, or a migration that ensures that the field is correctly defined (where it would only check for the value, and fail until manually fixed)

j-coll · 2025-01-21T16:07:48Z

.../main/java/org/opencb/opencga/analysis/clinical/exomiser/ExomiserInterpretationAnalysis.java

-                            logger.warn("Variant {} (normalizedToTsv {}), not found in map variantTranscriptMap", variant.toStringSimple(),
-                                    normalizedToTsv.get(variant.toStringSimple()));
+                            logger.warn("Variant {} (normalizedToTsv {}), not found in map variantTranscriptMap",
+                                    variant != null ? variant.toStringSimple() : null,


Why variant.toStringSimple() and not variant.toString() ??
You might get duplicated values with the first one

j-coll · 2025-01-21T16:33:12Z

opencga-catalog/src/main/java/org/opencb/opencga/catalog/utils/ResourceManager.java

+
+    private void loadConfiguration() throws IOException {
+        if (configuration == null) {
+            this.configuration = Configuration.load(new FileInputStream(openCgaHome.resolve(CONF_DIRNAME)


FileInputStream not being closed

Better use ConfigurationUtils.loadConfiguration

j-coll · 2025-01-21T16:34:46Z

opencga-catalog/src/main/java/org/opencb/opencga/catalog/utils/ResourceManager.java

+
+    protected static Logger logger = LoggerFactory.getLogger(ResourceManager.class);
+
+    public ResourceManager(Path openCgaHome) {


Why not having a builder with both opencgaHome and the configuration?

Most of the places (all that I've checked) calling this method do already have access to a configuration instance.

j-coll · 2025-01-21T16:52:43Z

opencga-app/src/main/java/org/opencb/opencga/app/cli/main/custom/CustomFilesCommandOptions.java

@@ -72,6 +72,8 @@ public class UploadCommandOptions {
                + "removed", arity = 0)
        public boolean replace;

+        @Parameter(names = {"--resource"}, description = "File resource", arity = 1)


Use ParamConstants.FILE_RESOURCE_DESCRIPTION

or, at least, its latest value

j-coll · 2025-01-21T17:01:12Z

opencga-core/src/main/java/org/opencb/opencga/core/api/FieldConstants.java

@@ -291,7 +291,8 @@ public class FieldConstants {
    public static final String FILE_CHECKSUM = "The checksum of the file.";
    public static final String FILE_PATH = "The path of the file.";
    public static final String FILE_URI = "The uri of the file.";
-    public static final String FILE_EXTERNAL = "Indicates the file is external or not.";
+    public static final String FILE_EXTERNAL = "Indicates whether the file comes from an external path or not.";
+    public static final String FILE_RESOURCE = "Indicates the file is treated as a resource.";


Isn't this param almost identical to the ParamConstant "FILE_RESOURCE_DESCRIPTION" ? Why?

j-coll · 2025-01-21T17:03:47Z

...pp/src/main/java/org/opencb/opencga/app/migrations/v4/v4_0_0/catalog/ResourcesMigration.java

+    protected void run() throws Exception {
+        MongoCollection<Document> fileCollection = getMongoCollection(OrganizationMongoDBAdaptorFactory.FILE_COLLECTION);
+
+        queryMongo(OrganizationMongoDBAdaptorFactory.STUDY_COLLECTION, new Document(),


No migration test?

j-coll · 2025-01-21T17:06:22Z

opencga-catalog/src/main/java/org/opencb/opencga/catalog/db/mongodb/StudyMongoDBAdaptor.java

+        FqnUtils.FQN fqn = FqnUtils.parse(study.getFqn());
+        String organization = fqn.getOrganization();
+        try {
+            URI studyUri = ioManager.getStudyUri(organization, Long.toString(project.getUid()), Long.toString(study.getUid()));


Understand...
I'd recommend creating a ticket to keep track of this, something like:
"Expose transactions API and remove IOManager from DBAdaptors"
The exposed transactions API should be only for internal use from the Managers, never from outside

j-coll · 2025-01-21T17:17:47Z

opencga-catalog/src/main/java/org/opencb/opencga/catalog/managers/FileManager.java

@@ -588,10 +585,16 @@ public OpenCGAResult<File> create(String studyStr, FileCreateParams createParams
            ParamUtils.checkParameter(createParams.getPath(), "path");
            ParamUtils.checkObj(createParams.getType(), "type");

+            boolean isResource = createParams.getResource() != null && createParams.getResource();


works the same and slightly shorter if you do createParams.getResource() == Boolean.TRUE

j-coll · 2025-01-22T14:45:23Z

opencga-core/src/main/java/org/opencb/opencga/core/config/AnalysisTool.java

@@ -26,13 +26,13 @@ public class AnalysisTool {
    private boolean defaultVersion;
    private String dockerId;
    private String params;
-    private Map<String, String> resources;
+    private List<String> resources;


This change will be problematic.. Should be well documented in the changelog

I'm definitely sure that we should have some sort of "configuration changes validation/migration" to ensure that we're not running with old non-migrated versions of the configuration.yml

j-coll · 2025-01-22T15:03:32Z

opencga-catalog/src/main/java/org/opencb/opencga/catalog/managers/FileManager.java

@@ -3443,6 +3523,10 @@ private OpenCGAResult<File> privateLink(String organizationId, Study study, File
                params.setPath(params.getPath() + "/");
            }
        }
+        if (isResourcesPath(params.getPath())) {
+            throw new CatalogException("Linked files cannot be stored in the RESOURCES folder.");


a Resource file can be uploaded but not linked? Why?

jtarraga and others added 30 commits October 3, 2024 15:01

core: add ResourceManager and JUnit tests, #TASK-7047, #TASK-6442

0ee51eb

catalog: create default RESOURCE folder for studies, #TASK-6442

c6f84ef

core: remove old FileCreateParams, #TASK-6442

77cccad

core: add an MD5 check when the resource manager downloads a file, #T…

b754a47

…ASK-7047, #TASK-6442

Merge branch 'TASK-6442' of https://github.com/opencb/opencga into TA…

7923785

…SK-6442

analysis: implement the tool DownloadResourcesTool to download all an…

6fb08b9

…alyisis resources for the current OpenCGA version, and generate clients, #TASK-7088, #TASK-6442

core: minor improvements

2fd7678

core: first download all resources to a temp folder, then copy them t…

0e80c4d

…o the installation folder, #TASK-7088, #TASK-6442

Merge branch 'TASK-6442' of https://github.com/opencb/opencga into TA…

8673ebd

…SK-6442

core: rename DownloadResourceTool to ResourceFetcherTool, #TASK-7088,…

3baf7c9

… #TASK-6442

master: call the ResourceFetcherTool by the monitor service on initia…

8885f30

…lization, and add the flag fetchResourceOnInit in the configuration file, #TASK-7088, #TASK-6442

client: generate clients after renaming some fields, #TASK-7088, #TAS…

774c286

…K-6442

core: use .md5 file to check if the resource file must be downloaded,…

2b6bfa0

… #TASK-7088, #TASK-6442

app: add migration to create missing RESOURCES folder, #TASK-6442

d6007c9

analysis: fetch all resources only is exececuted by admins, #TASK-708…

c4b872c

…8, #TASK-6442

Merge branch 'TASK-6442' of https://github.com/opencb/opencga into TA…

6629f3f

…SK-6442

catalog: add resource checks, #TASK-6442

90e7280

catalog: fix several tests, #TASK-6442

701a076

analysis: improve resource suppport and add resource exception, #TASK…

297009c

…-7088, #TASK-6442

Merge branch 'TASK-6442' of https://github.com/opencb/opencga into TA…

fa2cba7

…SK-6442

analysis: simplify ResourceManager code, #TASK-7047, #TASK-6442

4fad723

catalog: fix some tests, #TASK-6442

0791fe0

Merge branch 'TASK-6442' of github.com:opencb/opencga into TASK-6442

e1b5b70

analysis: update Exomiser analysis to use ResourceManager, #TASK-7125…

973d72d

…, #TASK-6442

Merge branch 'TASK-6442' of https://github.com/opencb/opencga into TA…

379a0bf

…SK-6442

analysis: add the action UNZIP to unzip Exomiser files after download…

9807f37

…ing, #TASK-7125, #TASK-6442

analysis: update mutational signature analysis according to the new r…

97ce73f

…esource management, #TASK-7127, #TASK-6442

core: change resource metadata to be independent/standalone, and rena…

ff513c2

…me some constants, #TASK-7088, #TASK-6442

analysis: remove ResourceUtils and update relatedness analysis by usi…

990b757

…ng the ResourceManager, #TASK-7139, #TASK-6442

analysis: fix exomiser resource path, #TASK-7125, #TASK-6442

159e76a

pfurio and others added 26 commits December 10, 2024 16:10

catalog: use constants where possible, #TASK-6442

70ee362

Merge branch 'develop' into TASK-6442

0eb2ad3

catalog: improve ResourceManager, #TASK-7047, #TASK-6442

9d8ddb2

catalog: remove unused import, #TASK-7047, #TASK-6442

e0d0f9f

analysis: update relatedness/family-qc according to resource manager …

eed1973

…changes, #TASK-7139, #TASK-6442

analysis: update exomiser according to resource manager changes, #TAS…

dc0de9c

…K-7125, #TASK-6442

analysis: update mutational signature according to resource manager c…

6e6141c

…hanges, #TASK-7127, #TASK-6442

analysis: update fetch resource tool according to resource manager ch…

44d3c3f

…anges, #TASK-7088, #TASK-6442

test: fix JUnit test and update configuration files, #TASK-7047, #TAS…

8aed078

…K-6442

Merge branch 'TASK-6442' of https://github.com/opencb/opencga into TA…

943e4de

…SK-6442

client: generate clients, #TASK-6442

a5cee2a

core: add relatedness resources in the configuration file, #TASK-6442

218510b

catalog: update JUnit tests for resource manager, #TASK-7047, #TASK-6442

0956608

master: use the admin study for the resource fetcher job, #TASK-7088,…

ad213ec

… #TASK-6442

core: complete the section resource in the configuration file, #TASK-…

f9347a8

…6442

server: rename endpoint resource/downloadAll to resource/fetch, #TASK…

a05d8dc

…-7088, #TASK-6442

catalog: try workaround for test env. failure, #TASK-6442

e162072

core: set resources in fetchOnInit in the configuration file, #TASK-6442

e157cd0

catalog: fix Exomiser resources in the configuration file, #TASK-7047…

152ec14

…, #TASK-6442

core: fix configuration files for Exomiser resources, #TASK-7047, #TA…

0c50eab

…SK-6442

analysis: update exomiser according to changes, #TASK-7125, #TASK-6442

134c540

analysis: fix Exomiser JUnit tests, #TASK-7125, #TASK-6442

c181ab4

catalog: remove Exomiser tmp files, #TASK-7125, #TASK-6442

6c2e33c

catalog: check if files exit before deleting them, #TASK-7047, #TASK-…

c833066

…6442

analysis: delay GRCh37 for Exomiser analysis and update the configura…

310f6d6

…tion file according, #TASK-7125, #TASK-6442

Merge branch 'develop' into TASK-6442

86109f3

jtarraga requested a review from j-coll December 20, 2024 09:30

Merge branch 'develop' into TASK-6442

02e3ae4

j-coll reviewed Jan 21, 2025

View reviewed changes

j-coll requested changes Jan 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TASK-6442 - Add Resources Management functionality #2535

TASK-6442 - Add Resources Management functionality #2535

pfurio commented Dec 10, 2024

j-coll left a comment

j-coll Jan 21, 2025

j-coll Jan 21, 2025

j-coll Jan 21, 2025

j-coll Jan 21, 2025

j-coll Jan 21, 2025

j-coll Jan 21, 2025

j-coll Jan 21, 2025

j-coll Jan 21, 2025

j-coll Jan 21, 2025

j-coll Jan 21, 2025

j-coll Jan 21, 2025

j-coll Jan 21, 2025

j-coll Jan 21, 2025

j-coll Jan 22, 2025

j-coll Jan 22, 2025

j-coll Jan 22, 2025


		protected static Logger logger = LoggerFactory.getLogger(ResourceManager.class);

		public ResourceManager(Path openCgaHome) {

TASK-6442 - Add Resources Management functionality #2535

Are you sure you want to change the base?

TASK-6442 - Add Resources Management functionality #2535

Conversation

pfurio commented Dec 10, 2024

j-coll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment