Handle different base paths explorer #427
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
PhilippeMoussalli
changed the title
| ENTRYPOINT [ "streamlit", "run", "/app/main.py", "--server.headless", "true", \ | ||
| "--server.fileWatcherType", "none", "--browser.gatherUsageStats", "false", \ | ||
| "--server.port=8501", "--server.address=0.0.0.0", "--"] |
There was a problem hiding this comment.
the '--' at the end is need to pass additional arguments (remote base path) as mentioned here
PhilippeMoussalli
requested review from
GeorgesLorre,
mrchtr and
ChristiaensBert
PhilippeMoussalli
force-pushed
the
data-explorer-handle-base-paths
branch
from
b1e6f13
to
02b2710
Compare
mrchtr
reviewed
Sep 13, 2023
data_explorer/app/filesystem.py
Outdated
There was a problem hiding this comment.
double of the src/fondant/filesystem.py ?
There was a problem hiding this comment.
nice catch, indeed this is not needed
First review #427 PR that updates the interface of the data explorer  Notes: * Right now the base path that we currently have has a mix of subsets and fields. Related to #244 * The current base paths contains a mix of subsets and pipelines due to the old way of organizing. In the future it should only include pipeline names. #368 (comment) Perhaps we can do some cleanup to our current base path and remove unnecessary subsets ``` gs://soy-audio-379412_kfp-artifacts/custom_artifact ```
| @@ -4,11 +4,12 @@ | |||
| import os | |||
| from typing import Dict, List, Optional, Tuple, Union | |||
|
|
|||
| from fondant.filesystem import get_filesystem | |||
There was a problem hiding this comment.
this import also needs a change I think ?
There was a problem hiding this comment.
no we still have src/filesystem.py so this should be fine
GeorgesLorre
approved these changes
Sep 18, 2023
Hakimovich99
pushed a commit
that referenced
this pull request
Oct 16, 2023
Linked to #426 #424 Previous implementation of the local runner could only work with a local runner and not the remote one. This PR enables the explorer to handle both. If the base path is local, it will be mounted to the explorer. Otherwise, the base path will be passed as a container argument inside the component Notes: * Cloud credentials are optional for the local runner (manifest is local but can point to a remote ) but mandatory with the remote base path
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.
Linked to #426 #424
Previous implementation of the local runner could only work with a local runner and not the remote one. This PR enables the explorer to handle both. If the base path is local, it will be mounted to the explorer. Otherwise, the base path will be passed as a container argument inside the component
Notes: