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
[DataCatalog2.0]:
KedroDataCatalog
#4151[DataCatalog2.0]:
KedroDataCatalog
#4151Changes from 155 commits
a8f4fb3
7d56818
787e121
0b80f23
5c727df
05c9171
5c804d6
e9ba5c4
530f7d6
64be83c
c29828a
957403a
14908ff
c5e925b
b9a92b0
2cb794f
2f32593
d1ea64e
fb89fca
4486939
c667645
be8e929
ec7ac39
8e23450
529e61a
e4cb21c
50bc816
6dfbcb0
9346f08
9568e29
86efdfe
5e27660
72b11d0
f0a4090
a4da52a
7d6227f
4092291
63e47f9
85bf720
68f6527
2ac4a2f
cb5879d
9038e96
cc89565
59b6764
4f5a3fb
12ed6f2
a106cec
6df04f7
8566e27
2dcea33
a46597f
3787545
68d612d
af5bee9
acc4d6e
e67ff0f
7b3afa2
658a759
7be2a8e
09f3f26
9e43a9a
b28a9bf
c9f3469
49a3b27
25b6501
3a646de
5e5df4a
aa59a35
c7efa3e
023ffc6
6971779
d57a567
585b44f
2769def
e447078
beb0165
975e968
11d782c
e4abd23
5f105de
f9cb9c6
31a9484
ced1b7a
7f9b576
a3828d9
16610c4
321affe
ff25405
d0000c0
7f5ddec
e030bb6
6433dd8
355576f
39d9ff6
659c9da
840b32a
77f551c
6e079a1
1f7e5f8
80f0e3d
017cda3
cab6f06
ac1ecc0
e955930
a07f3d4
4ecb826
2b9be66
fb3831b
8f604d1
9a4db18
0a6946a
fee7bd6
f5a7992
1c981f3
6128be7
8c91d0e
18d2ba0
d48c6d3
45ce6bc
6ca972f
fdce5ea
3029963
0150a21
0833a84
95ccb3c
07908a8
25a6fcf
07f8c12
3a1a0f2
cf663a0
caa7316
9540a32
0ac154d
0ec1f23
96d4576
4ecd8fd
11b3426
741b682
88ba38b
0020095
78feb51
6bf912c
c7699ec
bcd2d37
eb7e8f5
7348c12
5aee9e9
c9c7c9a
c66df33
2f1dcbd
4b8d90c
8f870a8
70dc177
ae7a271
135cb0e
ca4867c
4745f71
033a0b7
e74ffda
00af3ec
2de7ccb
8affed6
a52672e
84f249c
2548119
ac124e3
f62ed03
b65609f
17199ad
44c576e
6d5f094
e24b2a6
c8ef90f
d19941f
572594e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 4 in RELEASE.md
GitHub Actions / vale
[vale] RELEASE.md#L4
Raw output
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.
Should this be:
class KedroDataCatalog(CatalogProtocol)
?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 only need it in case we share some logic between the implementations which we intentionally don't want to do to keep all the implementations independent from the protocol class.
https://peps.python.org/pep-0544/#explicitly-declaring-implementation
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 think it we should do it in here, since it will make sure we adhere to it. While it's optional, it's beneficial at no cost for us. Other implementers do not need to extend it though.
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 don't mind, but the point was to clearly show that we do not have a shared logic and addition of new catalog does not require an explicit declaration.
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.
Shouldn't we resolve only if
ds_name not in self._datasets
? Or it's just to make the code a bit simpler?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, the point was to prevent you from complaining about nested ifs 😅 I moved it inside the condition now.
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.
No, it was just a question, either is fine :)
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.
Could we rearrange this in such a way that we fail first and then continue with the successful path? Currently the flow is as follows:
I think we can make the flow a bit less zig-zagy.
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.
Well, we can only fail after we try to resolve. Otherwise, you get one more layer of
if
as the logic needs to go inside theif fail [] else []
scenario.Now the logic is like 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.
What I meant is that we should have as a first line something like:
and then continue with the error scenario, and then go on with everything else. It'll be much easier to follow this way. Btw while checking if this is possible, I saw a problem in the resolver - it can fail even if it matches, but that should not happen.
kedro/kedro/io/catalog_config_resolver.py
Lines 149 to 156 in 5147dfb
This ☝️ should have been checked at the config_resolver init time, basically we should not allow to create a config_resolver with unresolvable configs or add invalid configs that cannot be resolved.
Also there's other changes like e.g.
resolve_dataset_pattern
should be justresolve_pattern
similar to all other public methods there, which never include the worddataset
(rightfully). Hopefully the resolver API is not released yet, so we can change it now.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.
resolve_pattern
method encapsulates this, not to expose this logic outside theconfig_resolver
. So we ask theconfig_resolver
to provide a config for a pattern without bothering about how it's happening inside. I don't think we need to move any resolution logic (including any specific checks) to the catalog level.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.
In any case, this is a minor thing, let's merge it in as it is and then we can always come back and simplify it.
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.
Maybe?
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 think we should keep it as it is in case someone is trying to save an unregistered dataset.
get_dataset()
will resolve it in case it's a pattern or validate that the dataset is not in the catalog.