feat(core): make update work with new storage #2304
Conversation
m-alisafaee
force-pushed
the
2257-new-renku-update
branch
from
37cb9a7
to
33631be
Compare
m-alisafaee
force-pushed
the
2257-new-renku-update
branch
from
33631be
to
ec3bc7b
Compare
m-alisafaee
force-pushed
the
2257-new-renku-update
branch
from
ec3bc7b
to
8ab311c
Compare
m-alisafaee
force-pushed
the
2257-new-renku-update
branch
from
8ab311c
to
cff3d7d
Compare
m-alisafaee
force-pushed
the
2257-new-renku-update
branch
from
86496f8
to
95be0ca
Compare
m-alisafaee
force-pushed
the
2257-new-renku-update
branch
from
95be0ca
to
97146c1
Compare
| all_activities = defaultdict(set) | ||
|
|
||
| def have_identical_inputs_and_outputs(activity1, activity2): | ||
| return sorted(u.entity.path for u in activity1.usages) == sorted( |
There was a problem hiding this comment.
I think set() instead of sorted() would work just as well. Not that it matters but there's not really a reason for sorting.
There was a problem hiding this comment.
I believe there is not clear way of comparing activities. The idea here was to cover cases like cat A A B > C and cat A B B > C.
renku/core/commands/update.py
Outdated
| if paths: | ||
| # NOTE: Add the activity to check if it also matches the condition | ||
| downstream_chains.append((activity,)) | ||
| downstream_chains = [c for c in downstream_chains if any(g.entity.path in paths for g in c[-1].generations)] |
There was a problem hiding this comment.
Can users ask Renku to update a whole folder? if so we'd need to check it here.
There was a problem hiding this comment.
Yes, they can. I've fixed this and added a test for it.
|
|
||
| if len(activities) > 1: | ||
| activity_collection = ActivityCollection(activities=activities) | ||
| activity_gateway.add_activity_collection(activity_collection) |
There was a problem hiding this comment.
It looks like the "activity-collections" only exists to enable some tests. it is a bit odd to me to store something in users' repositories that is only used for our tests, essentially littering the database with unneeded data.
I guess we could have a TestingActivityGateway(ActivityGateway) in tests/ and inject that for tests, to only have this index when testing code?
There was a problem hiding this comment.
ActivityCollection is to mark that these activities have been executed together as a result of an update or a rerun. So, it's not just for testing. I was not sure what other metadata we need to include here (specifically if we need a link in the Activity to its ActivityCollection if any). What do you think?
There was a problem hiding this comment.
a link from ActivityCollection to Activity makes sense to me. Best to discuss it with the KG team as well
| @@ -141,6 +141,9 @@ def workflow_graph(self): | |||
| workflow_graph.add_node(node) | |||
| continue | |||
|
|
|||
| if not next(self.graph.predecessors(node), None): | |||
There was a problem hiding this comment.
It might only make a difference on very large repositories with complex workflows, but
intermediate_predecessor = next(self.graph.predecessors(node), None)
if not intermediate_predecessor:
continue
[...]
source = next(self.graph.predecessors(intermediate_predecessor), None)
would only have to calculate the predecessor once.
There was a problem hiding this comment.
It always helps with the code's readability.
| name: str, | ||
| derived_from: str = None, | ||
| plans: List[Union["CompositePlan", Plan]] = None, | ||
| plans: List[Plan] = None, |
There was a problem hiding this comment.
Should it be AbstractPlan ? A CompositePlan created by a user could contain a CompositePlan
There was a problem hiding this comment.
👍
I made it as it was before (Union["CompositePlan", Plan]) which should also help with ide type linter.
m-alisafaee
force-pushed
the
2257-new-renku-update
branch
6 times, most recently
from
45f3998
to
7279445
Compare
m-alisafaee
force-pushed
the
2257-new-renku-update
branch
from
7279445
to
f57ccb2
Compare
Description
renku updatewith the new storage/metadata. It has aPATHSparameter which limits the update to the specified paths.Fixes #2257