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

refactor(deploy): Harmonize docker mount points #154

Merged
merged 4 commits into from
Feb 23, 2024

Conversation

AlexAxthelm
Copy link
Collaborator

Change from /outputs to /mnt/outputs, to match structure defined in #142 and #145

No associated issue.

Change from /outputs to /mnt/outputs, to match structure defined in #142 and #145

No associated issue.
@AlexAxthelm AlexAxthelm requested a review from jdhoffa February 18, 2024 17:26
@AlexAxthelm AlexAxthelm mentioned this pull request Feb 18, 2024
6 tasks
@cjyetman
Copy link
Member

Gonna take me some time to test this locally to make sure it works in the local Docker context. Some quick thoughts though...

  1. I don't see any obvious logical explanation for why the outputs path is under /mnt while the inputs directories are not.
  2. I will be hyper-critical of any changes to config.yml, and in particular, in order of decreasing severity, changes to the "default" config and changes to the default timestamp configs, e.g. "2022Q4"
  3. I'm leaning towards not defining paths in any of the default configs, since the paths are always context specific, so in the near future this will probably look more like...
default:
  data_prep_outputs_path: ""
  asset_impact_data_path: ""
  factset_data_path: ""

2022Q4_azure:
  inherits: 2022Q4
  data_prep_outputs_path: "/mnt/outputs"
  asset_impact_data_path: "/inputs"
  factset_data_path: "/inputs"

2022Q4_docker:
  inherits: 2022Q4
  data_prep_outputs_path: "/outputs"
  asset_impact_data_path: "/inputs"
  factset_data_path: "/inputs"

2022Q4_desktop:
  inherits: 2022Q4
  data_prep_outputs_path: "/path/to/local/outputs"
  asset_impact_data_path: "/path/to/local/inputs"
  factset_data_path: "/path/to/local/inputs"

@cjyetman
Copy link
Member

prefer to wait until #161 merges and add this new context specific path to the new "docker" config instead

Copy link
Member

@cjyetman cjyetman left a comment

Choose a reason for hiding this comment

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

tested, seems to work... still uncertain about the logic of outputs living under /mnt but inputs not

@cjyetman cjyetman merged commit 9fbac2c into main Feb 23, 2024
@cjyetman cjyetman deleted the harmonize-output-filepath-for-docker branch February 23, 2024 08:15
@AlexAxthelm
Copy link
Collaborator Author

This was compensating for the fact that I forgot to move the outputs directory in #145

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants