feature/derive_local_path #389

BrianWhitneyAI · 2025-01-10T23:14:48Z

Description

The purpose of this PR is to resolve #390. When working with the ETL it became apparent that having an annotation with a unique value for each file puts a lot of stress on the annotation collection. We recently added local file path and Cache Eviction Date to this collection which caused a breakage of our annotation collection rebuild.

The solution we came up with is to remove the local path annotation and derive it from fms ID and show conditionally based on the presence of Cache Eviction Date. Additionally Cache Eviction Date will move to a bucketed date time, decreasing the unique values for files and making the annotation more palatable.

This PR also removes the previous backwards compatibility.

BrianWhitneyAI · 2025-01-13T20:29:56Z

Philip brought up that we could derive from the Cloud path as well which would be much simpler. I opted against this idea since the cloud path is already derived from fms-ID. Open to a discussion here!

SeanLeRoy · 2025-01-13T20:53:11Z

packages/core/entity/FileDetail/index.ts

@@ -85,12 +87,69 @@ export default class FileDetail {
        return FileDetail.convertAicsS3PathToHttpUrl(`${AICS_FMS_S3_BUCKET}${pathWithoutDrive}`);
    }

+    private static generateFilePath(env: Environment, fileName: string, fmsId: string): string {


These parameters are available on the object so by making it static you make more work for the caller and this method

Resolved by deriving from cloud path in 3bee712

SeanLeRoy · 2025-01-13T20:53:47Z

packages/core/entity/FileDetail/index.ts

+        }
+
+        // Define prefixes based on the environment
+        const prefixes: Record<Environment, string> = {


This constant can live outside the declaration of this class (just right above where the class is declared)

resolved in 3bee712

packages/core/entity/FileDetail/index.ts

SeanLeRoy · 2025-01-13T20:55:31Z

packages/core/entity/FileDetail/index.ts

+        };
+
+        // Get the prefix for the given environment
+        const prefix = prefixes[env];


prefix feels non-descript - perhaps "nasDirectory"

envToPrefix ?

SeanLeRoy · 2025-01-13T20:56:32Z

packages/core/entity/FileDetail/index.ts

+        return filePath;
+    }
+
+    private static convertFMSIDToLocalPath(guid: string): string[] {


This has basically the same name as the above method with a pretty different purpose - this sounds more like divideFmsIdIntoSegments

Same issue where because this is static you had to pass an accessible class property - also guid conflicts with the name of the method I'd stick with fileId since its used across the repo

Resolved by removing in 3bee712

SeanLeRoy · 2025-01-13T20:59:37Z

packages/core/entity/FileDetail/index.ts

+    }
+
+    private static convertFMSIDToLocalPath(guid: string): string[] {
+        if (!guid) {


Implicit as part of type

resolved by removing in 3bee712

SeanLeRoy · 2025-01-13T21:13:28Z

packages/core/entity/FileDetail/index.ts

+
+        const paths: string[] = [];
+
+        while (guid.length > 2) {


This is great, but FWIW lodash can do this whole method in one line for ya:

// Splits the fileId into an array where is character is its own element then chunks them into groups of 3 with the // remainder (between 1-3 characters) becoming the final group chunk(this.fileId.split(""), 3);

oh yeah I definitely just ported it over from FSS and didnt consider.

less custom code to test FTW

Resolved by deriving from cloud path in 3bee712

SeanLeRoy · 2025-01-13T21:14:47Z

packages/core/entity/FileDetail/index.ts

    private static convertAicsS3PathToHttpUrl(path: string): string {
        return `${AICS_FMS_S3_URL_PREFIX}${path}`;
    }

-    constructor(fileDetail: FmsFile, uniqueId?: string) {
+    constructor(fileDetail: FmsFile, env: Environment, uniqueId?: string) {


I think it'll be cleaner long-term to keep this out of the properties and instead pass it to a getter method getLocalPath() - this would replace the property localPath (FWIW localPath seen below is a property because of the get before its method name

I made getLocalPath as a function in 3bee712. However I left get localPath where it calls getLocaPath() since it has a conditional of Cache eviction date. Lmk what you think about this, open to just removing it and moving the conditional into getLocalPath()

packages/core/components/FileList/test/LazilyRenderedThumbnail.test.tsx

hughes036 · 2025-01-14T21:32:42Z

packages/core/constants/index.ts

@@ -31,6 +31,12 @@ export const TOP_LEVEL_FILE_ANNOTATIONS = [
        description: "Path to file in the cloud.",
        type: AnnotationType.STRING,
    }),
+    new Annotation({


Looks like this is the pattern for dealing with file properties?

Not sure what the comment is here but my interpretation is this format allows us to create our own annotations in BFF which we later propagate with the file properties.

hughes036 · 2025-01-14T21:38:35Z

packages/core/entity/FileDetail/index.ts

@@ -85,12 +87,69 @@ export default class FileDetail {
        return FileDetail.convertAicsS3PathToHttpUrl(`${AICS_FMS_S3_BUCKET}${pathWithoutDrive}`);
    }

+    private static generateFilePath(env: Environment, fileName: string, fmsId: string): string {


Does generateFilePath have test cov?

Not any more.... removed in 3bee712

hughes036 · 2025-01-14T21:50:05Z

packages/core/entity/FileDetail/index.ts

    }

    public get cloudPath(): string {
-        // REMOVE THIS (BACKWARDS COMPAT)
-        if (this.path.startsWith("/allen")) {


We still need backward compatibility - should this change (and the block below) use the new heuristic this.getAnnotation("Cache Eviction Date")?

The backwards compatibility is to handle file.path being '/allen' or the updated version where file.path = cloud path. I thought that we were fully past that after the new file rebuild, hence trying to remove it. however seems like there's still a possibility of file.path being /allen as talked about today in #cloud-first-fms so I am in favor of adding it back

In regards to Cache Eviction Date I don't think that we should use this yet since it has not been backfilled and will only show up on new annotations. We would not reach a state where the file path property was old where we had an eviction date I don't think?

hughes036 · 2025-01-14T21:52:52Z

packages/core/services/HttpServiceBase/index.ts

@@ -167,6 +167,19 @@ export default class HttpServiceBase {
        return new RestServiceResponse(cachedResponseData);
    }

+    public getEnvironmentFromUrl(): Environment {


has unit test cov?

removed in 3bee712

hughes036

See questions about backward compatibility. Lgtm otherwise.

BrianWhitneyAI and others added 4 commits January 10, 2025 15:14

feature/derive_local_path

a235a91

insert real fms id

80ae4fb

Fix up local path mount point test

e41c9bf

make localPath dependent on eviction

bfb0748

BrianWhitneyAI marked this pull request as ready for review January 13, 2025 20:30

Add unit test for file detail functions

c492acc

BrianWhitneyAI requested review from pgarrison, SeanLeRoy, aswallace, saeliddp, hughes036 and kmitcham and removed request for pgarrison and SeanLeRoy January 13, 2025 20:43

add back comment

c268675

SeanLeRoy requested changes Jan 13, 2025

View reviewed changes

hughes036 reviewed Jan 14, 2025

View reviewed changes

packages/core/components/FileList/test/LazilyRenderedThumbnail.test.tsx Show resolved Hide resolved

hughes036 reviewed Jan 14, 2025

View reviewed changes

hughes036 approved these changes Jan 14, 2025

View reviewed changes

derive path from cloudpath

3bee712

BrianWhitneyAI requested a review from SeanLeRoy January 15, 2025 00:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature/derive_local_path #389

feature/derive_local_path #389

BrianWhitneyAI commented Jan 10, 2025 •

edited

Loading

BrianWhitneyAI commented Jan 13, 2025

SeanLeRoy Jan 13, 2025

BrianWhitneyAI Jan 15, 2025

SeanLeRoy Jan 13, 2025

BrianWhitneyAI Jan 15, 2025

SeanLeRoy Jan 13, 2025

hughes036 Jan 14, 2025

SeanLeRoy Jan 13, 2025

SeanLeRoy Jan 13, 2025

BrianWhitneyAI Jan 15, 2025

SeanLeRoy Jan 13, 2025

BrianWhitneyAI Jan 15, 2025

SeanLeRoy Jan 13, 2025

BrianWhitneyAI Jan 14, 2025

hughes036 Jan 14, 2025

BrianWhitneyAI Jan 15, 2025

SeanLeRoy Jan 13, 2025

BrianWhitneyAI Jan 15, 2025

hughes036 Jan 14, 2025

BrianWhitneyAI Jan 15, 2025

hughes036 Jan 14, 2025

BrianWhitneyAI Jan 15, 2025

hughes036 Jan 14, 2025

BrianWhitneyAI Jan 15, 2025

BrianWhitneyAI Jan 15, 2025

hughes036 Jan 14, 2025

BrianWhitneyAI Jan 15, 2025

hughes036 left a comment

feature/derive_local_path #389

Are you sure you want to change the base?

feature/derive_local_path #389

Conversation

BrianWhitneyAI commented Jan 10, 2025 • edited Loading

Description

BrianWhitneyAI commented Jan 13, 2025

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

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

hughes036 left a comment

Choose a reason for hiding this comment

BrianWhitneyAI commented Jan 10, 2025 •

edited

Loading