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.
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
Unified file source stage #1184
base: branch-23.11
Are you sure you want to change the base?
Unified file source stage #1184
Changes from 6 commits
acfd425
dca5e91
d2f8ff8
51cdc0c
32883c0
2dd8d89
5aa68e5
bc73877
f27ea80
ea56c5f
0474b96
7bd1584
e6093ec
0feb848
c01c248
9a6b9e6
f5d0510
54e1f5c
bf953b6
e83edee
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Remark: Several pieces of functionality diverge depending on whether or not we have local or remote files. It makes me wonder if we actually need separate classes for local/remote cases. It also begs the question: what happens if some of the files are local and some are remote?
Remark: My hunch is that what we really want is:
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.
We perform a validation step for remote host paths, selecting the fsspec route if they exist, and subsequently confirming that they share the same protocol; otherwise, an error is raised.
It is implemented based on the description mentioned in this issue: #976
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.
Remark: Without breaking this Stage in to multiple Stages as mentioned in a separate comment, implementing this Stage in C++ might be a big headache.
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.
Yeah, let's discuss about this
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.
Question: @mdemoret-nv Is it possible this prevents proper shutdown? Should we add a cancellation token?
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.
I have an updated version to request shutdown by calling stop method.
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.
@cwharris you are right, the updated version didn't resolve the issue. The pipeline terminated abruptly with Ctrl+C only a few times, possibly due to unrelated factors. However, in general, it's still not shutting down properly.
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.
I haven't knowingly witnessed a clean shutdown of a Morpheus pipeline through the use of ctrl+c. I should check to see if that's even possible (it should be, but might have limits).