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
Extend layout placeholder params for filesystem destinations #1149
Extend layout placeholder params for filesystem destinations #1149
Changes from 37 commits
d64dfc0
a21ea39
21dda29
f017465
929b0e5
e7b4636
6117a20
43f0634
1713e4e
27604b9
b34da5d
9d7d763
9613ce3
a787c2a
0c5d58f
834b802
ccb5d82
f5479e4
e85345f
7cc7954
f9c8955
247cbca
c1e6644
e4609bd
d23f51f
ab5504d
c773c65
96f83e1
14beaef
3bf1e42
39c2ad7
ac1a7d2
ae2f826
bb792a8
9afbdcd
f3af8f5
bac2e6b
72c3f5a
6b7a008
cf37c66
be5ca1d
33671b4
fa295c6
a0cad82
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.
extra_placeholders
would be betterThere 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.
you do not need this method at all. config will be populated automatically
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.
OK you need it to validate the layout. at this moment you know all the config values. some come from providers some from factory,
so here you need to validate the layout. see if all placeholders are known. if not we need a really good explanation which are missing etc.
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.
here you should have the same typing as in config
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 am passing these three arguments via
kwargs
so here on resolve I reconcile and can override the values fromconfig.toml
.It doesn't feel right however is it fine @sh-rp @rudolfix ?
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.
OK
factory
is tricky and its main goal is to delay instantiation, population of the configs until runtime (ie. load step) while the UX is like you can create destination at any moment.look at base init
it will store all unknown paramters in
config_params
and then here
whenever configuration is created, we use those params as explicit initials
outcome: you do not need any special code to handle that. just pass additional stuff to super() init and it will be pushed to config during resolution
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.
Done.
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.
don't do that.
fsspec
does that input_file
. this code is not needed. what was your reason to add it?OFC this smells like race conditions that's why we try to "create" dirs in initialize. btw. buckets do not have dirs really
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.
@rudolfix the strange thing was that ffspec didn't create directory that's why I added it, I will remove this and see if test pipeline will work out.
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.
not needed
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'll comment on that in next review. this is a bigger chunk. we want to support all paths in append-only and require old layout only when we replace data (we can't guarantee replacement otherwise)