-
Notifications
You must be signed in to change notification settings - Fork 7
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
Set correct path for uploaded file in sim config #1054
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mattdailis
temporarily deployed
to
test-workflow
December 11, 2023 18:37 — with
GitHub Actions
Inactive
mattdailis
temporarily deployed
to
test-workflow
December 11, 2023 18:47 — with
GitHub Actions
Inactive
mattdailis
temporarily deployed
to
test-workflow
December 11, 2023 18:51 — with
GitHub Actions
Inactive
mattdailis
force-pushed
the
fix/upload-files-sim-config
branch
from
December 11, 2023 21:44
48dd6af
to
5a58388
Compare
mattdailis
temporarily deployed
to
test-workflow
December 11, 2023 21:44 — with
GitHub Actions
Inactive
duranb
requested changes
Dec 13, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good for the most part other than one comment that I left!
mattdailis
force-pushed
the
fix/upload-files-sim-config
branch
from
December 13, 2023 17:28
5a58388
to
284070b
Compare
mattdailis
temporarily deployed
to
test-workflow
December 13, 2023 17:28 — with
GitHub Actions
Inactive
mattdailis
temporarily deployed
to
test-workflow
December 13, 2023 19:49 — with
GitHub Actions
Inactive
mattdailis
temporarily deployed
to
test-workflow
December 13, 2023 19:55 — with
GitHub Actions
Inactive
mattdailis
temporarily deployed
to
test-workflow
December 13, 2023 20:35 — with
GitHub Actions
Inactive
mattdailis
temporarily deployed
to
test-workflow
December 13, 2023 20:42 — with
GitHub Actions
Inactive
duranb
reviewed
Dec 13, 2023
mattdailis
temporarily deployed
to
test-workflow
December 13, 2023 22:40 — with
GitHub Actions
Inactive
duranb
approved these changes
Dec 13, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 🥇 🚀
JosephVolosin
pushed a commit
that referenced
this pull request
Aug 20, 2024
* Set correct path for uploaded file in sim config * Make eslint happy * Undo unintentional change * Provide type for filenames * Add unit test and fix bug found by unit test * Alphabetically sort keys for eslint * Add PUBLIC_AERIE_FILE_STORE_PREFIX env var * Reduce use of continue where simple if statement suffices * Rename filenames to generatedFilenames and pathsToReplace
JosephVolosin
pushed a commit
that referenced
this pull request
Oct 21, 2024
* Set correct path for uploaded file in sim config * Make eslint happy * Undo unintentional change * Provide type for filenames * Add unit test and fix bug found by unit test * Alphabetically sort keys for eslint * Add PUBLIC_AERIE_FILE_STORE_PREFIX env var * Reduce use of continue where simple if statement suffices * Rename filenames to generatedFilenames and pathsToReplace
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #1046
Description
Aerie allows the mission model to declare a sim config parameter of type
Path
. The Aerie UI displays this parameter type with aBrowse
button that allows the user to upload a file.The aerie gateway changes the names of uploaded files to ensure uniqueness (e.g. if I upload
banananation.jar
multiple times, I do not typically want it to overwrite other files with the same name).The Aerie UI sets the name of the
Path
parameter in the sim config to the name of the original file, not the modified name generated by the gateway. This path also omits the absolute path prefix of the location at which theaerie_file_store
is mounted in theaerie_merlin
andaerie_merlin_worker_*
containers.As a consequence of the above, there is no way for the mission model to take the provided
Path
parameter and resolve it to the correct file location in the file system. This negates the utility of usingPath
parameters in a sim config.Problem Statement
A mission model should be able to use a sim config parameter of type
Path
to find a file that the planner uploaded using the UI's sim config form.Approach
The most targeted fix (i.e. a fix that only touches one repo) is to update the UI to fill in the Path parameter with the correct absolute path to the uploaded file. It can do this with the following steps:
Path
parameters and replace them with a hard-coded prefix followed by the generated filename, e.g:A drawback of this approach is that it hard-codes the mount point
/usr/src/app/merlin_file_store/
. Perhaps an easy step we could take would be to set this in an environment variable./usr/src/app/merlin_file_store/
in an environment variableVerification
As a manual test, upload a banananation mission model, and upload a text file to the
initialDataPath
sim config parameter. Run a simulation, and observe the/data/line_count
resource - it should correctly report the number of lines in your uploaded file.Future work
merlin_file_store
feels a bit strange - in theory, the simulation context should be able to add that prefix to every provided path parameter.