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

367: Remove Canonical path and format Cloud path #375

Open
wants to merge 4 commits into
base: feature/368-use-local-file-path-annotation
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 6 additions & 19 deletions packages/core/components/FileDetails/FileAnnotationList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,12 @@ export default function FileAnnotationList(props: FileAnnotationListProps) {
}
}

const ret = [
if (annotation.name === AnnotationName.FILE_PATH) {
// Display the full http://... URL
annotationValue = fileDetails.cloudPath;
}

return [
...accum,
<FileAnnotationRow
key={annotation.displayName}
Expand All @@ -97,24 +102,6 @@ export default function FileAnnotationList(props: FileAnnotationListProps) {
value={annotationValue}
/>,
];

// Special case for file paths: we want to display both the "canonical" FMS path
// (i.e. POSIX path held in the database; what we have an annotation for)
// as well as the path at which the file is *actually* accessible on _this_ computer ("local" file path)
if (annotation.name === AnnotationName.FILE_PATH) {
if (fileDetails.path !== fileDetails.cloudPath) {
ret.push(
<FileAnnotationRow
key="file-path-cloud"
className={styles.row}
name="File Path (Cloud)"
value={fileDetails.cloudPath}
/>
);
}
}

return ret;
}, [] as JSX.Element[]);
}, [annotations, fileDetails, isLoading, localPath]);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { AnnotationType } from "../../../entity/AnnotationFormatter";

describe("<FileAnnotationList />", () => {
describe("file path representation", () => {
it("has both canonical file path and file path adjusted to OS & allen mount point", async () => {
it("has both cloud file path and local file path adjusted to OS & allen mount point", async () => {
// Arrange
const hostMountPoint = "/some/path";

Expand Down Expand Up @@ -46,7 +46,7 @@ describe("<FileAnnotationList />", () => {
});

const filePathInsideAllenDrive = "path/to/MyFile.txt";
const filePath = `production.allencell.org/${filePathInsideAllenDrive}`;
const filePath = `production.files.allencell.org/${filePathInsideAllenDrive}`;
const fileDetails = new FileDetail({
file_path: filePath,
file_id: "abc123",
Expand All @@ -58,8 +58,6 @@ describe("<FileAnnotationList />", () => {
],
});

const localPath = `${hostMountPoint}/${filePathInsideAllenDrive}`;

// Act
const { findByText } = render(
<Provider store={store}>
Expand All @@ -69,16 +67,16 @@ describe("<FileAnnotationList />", () => {

// Assert
for (const cellText of [
"File Path (Canonical)",
filePath,
"File Path (Cloud)",
`https://s3.us-west-2.amazonaws.com/${filePath}`,
"File Path (Vast)",
localPath,
`${hostMountPoint}/${filePathInsideAllenDrive}`,
]) {
expect(await findByText(cellText)).to.not.be.undefined;
}
});

it("has only canonical file path when no allen mount point is found", () => {
it("has only cloud file path when no allen mount point is found", () => {
// Arrange
class FakeExecutionEnvService extends ExecutionEnvServiceNoop {
public formatPathForHost(posixPath: string): Promise<string> {
Expand All @@ -100,7 +98,7 @@ describe("<FileAnnotationList />", () => {
});

const filePathInsideAllenDrive = "path/to/MyFile.txt";
const filePath = `/allen/${filePathInsideAllenDrive}`;
const filePath = `production.files.allencell.org/${filePathInsideAllenDrive}`;
const fileDetails = new FileDetail({
file_path: filePath,
file_id: "abc123",
Expand All @@ -119,9 +117,11 @@ describe("<FileAnnotationList />", () => {

// Assert
expect(() => getByText("File Path (Vast)")).to.throw();
["File Path (Canonical)", filePath].forEach((cellText) => {
expect(getByText(cellText)).to.not.be.undefined;
});
["File Path (Cloud)", `https://s3.us-west-2.amazonaws.com/${filePath}`].forEach(
(cellText) => {
expect(getByText(cellText)).to.not.be.undefined;
}
);
});
});
});
4 changes: 2 additions & 2 deletions packages/core/constants/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ export const TOP_LEVEL_FILE_ANNOTATIONS = [
type: AnnotationType.STRING,
}),
new Annotation({
annotationDisplayName: "File Path (Canonical)",
annotationDisplayName: "File Path (Cloud)",
annotationName: AnnotationName.FILE_PATH,
description: "Path to file in storage.",
description: "Path to file in the cloud.",
type: AnnotationType.STRING,
}),
new Annotation({
Expand Down
24 changes: 17 additions & 7 deletions packages/core/entity/FileDetail/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ import { renderZarrThumbnailURL } from "./RenderZarrThumbnailURL";

const RENDERABLE_IMAGE_FORMATS = [".jpg", ".jpeg", ".png", ".gif"];

const AICS_FMS_S3_BUCKET = "production.files.allencell.org";
const AICS_FMS_S3_URL_PREFIX = "https://s3.us-west-2.amazonaws.com/";

/**
* Expected JSON response of a file detail returned from the query service. Example:
* {
Expand Down Expand Up @@ -79,7 +82,11 @@ export default class FileDetail {
const pathWithoutDrive = path.replace("/allen/programs/allencell/data/proj0", "");
// Should probably record this somewhere we can dynamically adjust to, or perhaps just in the file
Copy link
Contributor

Choose a reason for hiding this comment

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

agree with comment

// document itself, alas for now this will do.
return `https://s3.us-west-2.amazonaws.com/production.files.allencell.org${pathWithoutDrive}`;
return FileDetail.convertAicsS3PathToHttpUrl(`${AICS_FMS_S3_BUCKET}${pathWithoutDrive}`);
}

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

constructor(fileDetail: FmsFile, uniqueId?: string) {
Expand Down Expand Up @@ -116,6 +123,14 @@ export default class FileDetail {
if (path === undefined) {
throw new Error("File Path is not defined");
}

// AICS FMS files have paths like this in fileDetail.file_path:
// staging.files.allencell.org/130/b23/bfe/117/2a4/71b/746/002/064/db4/1a/danny_int_test_4.txt
if (typeof path === "string" && path.startsWith(AICS_FMS_S3_BUCKET)) {
return FileDetail.convertAicsS3PathToHttpUrl(path) as string;
}

// Otherwise just return the path as is and hope for the best
return path as string;
}

Expand All @@ -128,12 +143,7 @@ export default class FileDetail {
}

public get cloudPath(): string {
// Can retrieve a cloud like path for AICS FMS files
if (this.path.startsWith("/allen")) {
return FileDetail.convertAicsDrivePathToAicsS3Path(this.path);
}

// Otherwise just return the path as is and hope for the best
// AICS FMS files' paths are cloud paths
return this.path;
}

Expand Down
Loading