feat: remove separated workspace param #1722

florian-hoenicke · 2021-01-18T20:16:55Z

Breaking Change #1606

related discussion:
#1720

Changes introduced

Remove separate_workspace
Remote pea_workspace meta
Set workspace metas default to None to allow understand the logic where the user does not explicitly provides a value. Useful to clarify workspace structure in the case of CompoundExecutors. (In CompoundExecutor, components will compute its workspace considering some logic on its CompoundExecutor continent.
Have shard_workspace property to be defined functionally depending on the workspace, the name and the pea_id.
Adapt logic to set component workspace based on CompoundExecutor
Set root_workspace and root_name to set to root executor needed for CompoundExecutor and ref_indexer.
Have strong set of tests for different configurations of workspaces (Single, with Compound, with Compound but with specific workspace for its components, with ref_indexer, with ref_indexer inside a Compound
Use complete_path to find uses_internal resource to bind into docker when spinning a pea in a container

JoanFM

maybe we need to remove pea_workspace as well

jina/executors/metas.py

jina/jaml/parsers/executor/legacy.py

jina/parsers/peapods/pea.py

github-actions · 2021-01-18T20:28:47Z

Latency summary

Current PR yields:

🐢🐢 index QPS at 1140, delta to last 3 avg.: -21%
😶 query QPS at 26, delta to last 3 avg.: +3%

Breakdown

Version	Index QPS	Query QPS
current	1140	26
`0.9.18`	1454	25

Backed by latency-tracking. Further commits will update this comment.

…move-separated-workspace-param # Conflicts: # daemon/api/endpoints/flow.py # jina/peapods/pods/__init__.py

codecov · 2021-01-20T09:42:50Z

Codecov Report

Merging #1722 (260fbd0) into master (650f87e) will decrease coverage by 10.70%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master    #1722       +/-   ##
===========================================
- Coverage   85.18%   74.48%   -10.71%     
===========================================
  Files         134      134               
  Lines        6865     6878       +13     
===========================================
- Hits         5848     5123      -725     
- Misses       1017     1755      +738

Impacted Files	Coverage Δ
jina/enums.py	`95.91% <ø> (ø)`
jina/executors/metas.py	`96.87% <ø> (ø)`
jina/peapods/pods/helper.py	`96.80% <ø> (-0.10%)`	⬇️
jina/peapods/runtimes/zmq/zed.py	`84.89% <ø> (-6.48%)`	⬇️
jina/executors/compound.py	`77.86% <100.00%> (-7.74%)`	⬇️
jina/executors/indexers/vector.py	`87.04% <100.00%> (-6.08%)`	⬇️
jina/jaml/parsers/executor/legacy.py	`88.23% <100.00%> (+1.13%)`	⬆️
jina/parsers/peapods/pea.py	`100.00% <100.00%> (ø)`
jina/types/request/common.py	`0.00% <0.00%> (-100.00%)`	⬇️
jina/optimizers/flow_runner.py	`0.00% <0.00%> (-100.00%)`	⬇️
... and 58 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 650f87e...a579aaa. Read the comment docs.

JoanFM

Not ready to be merged, we need to add some set of integration tests

cristianmtr · 2021-01-21T17:02:10Z

jina/executors/__init__.py

+        """ Property to access `workspace` if existing or default to `./`. Useful to provide good interface when
+        using executors directly in python.
+
+        .. highlight:: python


This snippet doesn't seem relevant

What would you change?

I'd remove it, right?

dismiss review but not approving as part of the ticket

…move-separated-workspace-param # Conflicts: # jina/parsers/peapods/pea.py

…move-separated-workspace-param # Conflicts: # tests/integration/incremental_indexing/test_incremental_indexing.py

maximilianwerk

Good job, great refactoring. Just one point. I'd be happy to be convinced, that your implementation is better than my suggestion. :)

maximilianwerk · 2021-01-22T07:25:00Z

jina/executors/__init__.py

+
+        :return: returns the workspace property of the executor or default to './'
+        """
+        return self.workspace or './'


I am totally surprised, that this works.
After digging around a little: Seems like I never thought about how boolean expressions work in python. I am totally surprised, that

a = 'a' or 'b' => a = 'a' (First `True` evaluated element) a = 'a' and 'b' => a = 'b' (Last `True` evaluated element) a = '' or 'b' => a = 'b' (First `True` evaluated element, which is the second one) a = '' and 'b' => a = '' (First `False` evaluated element)

When you think how and and or are implemented (via __and__ and __or__) this makes total sense. I am just amazed... A consequence is:

def my_func(): return 'haha' if ['a'] and my_func(): ....

breaks down to

if 'haha': ...

which is obviously True.

maximilianwerk · 2021-01-22T07:27:36Z

jina/executors/__init__.py

+    @staticmethod
+    def get_shard_workspace(workspace_folder: str, workspace_name: str, pea_id: int) -> str:
+        # TODO (Joan, Florian). We would prefer not to keep `pea_id` condition, but afraid many tests rely on this
+        return os.path.join(workspace_folder, f'{workspace_name}-{pea_id}') if pea_id > 0 else os.path.join(workspace_folder)


couldn't it be just ... else workspace_folder?

And furthermore, if the whole line would just be

if pea_id < 0: pea_id = 0 return os.path.join(workspace_folder, f'{workspace_name}-{pea_id}')

we might be able to heavily simplify the parsers, non? If we have shards, it makes total sense, that we have the pea_id in the workspace name. But if we don't have shards but only one Pea, it still makes total sense to add the pea_id to the workspace name (in my opinion).

couldn't it be just ... else workspace_folder?

oh sure!!

About the second point, I agree, but I was afraid that it would break current logic and a huge amount of tests, and thus we decided to keep it like this as in the TODO is expressd

Ah I should read the comments as well... That's why I almost never write comments: I also never read them :D

Fine I'll approve

JoanFM · 2021-01-22T07:43:49Z

Good job, great refactoring. Just one point. I'd be happy to be convinced, that your implementation is better than my suggestion. :)

Your suggestion is definitely better, we went conservative on the amount of things we would break

JoanFM · 2021-01-22T07:44:08Z

Good job, great refactoring. Just one point. I'd be happy to be convinced, that your implementation is better than my suggestion. :)

Your suggestion is definitely better, we went conservative on the amount of things we would break

I would suggest to give it a try in a different PR

jina/executors/__init__.py

maximilianwerk

LGTM

feat: remove separated workspace param

5af8fcb

florian-hoenicke mentioned this pull request Jan 18, 2021

test: sharding workspace #1720

Closed

JoanFM reviewed Jan 18, 2021

View reviewed changes

jina/executors/metas.py Outdated Show resolved Hide resolved

JoanFM requested changes Jan 18, 2021

View reviewed changes

jina/jaml/parsers/executor/legacy.py Outdated Show resolved Hide resolved

jina/parsers/peapods/pea.py Outdated Show resolved Hide resolved

JoanFM closed this Jan 20, 2021

JoanFM reopened this Jan 20, 2021

Merge branch 'master' of https://github.com/jina-ai/jina into feat-re…

0d1300d

…move-separated-workspace-param # Conflicts: # daemon/api/endpoints/flow.py # jina/peapods/pods/__init__.py

fix: submodule

49fd3f3

florian-hoenicke marked this pull request as ready for review January 20, 2021 10:01

florian-hoenicke requested review from hanxiao and a team as code owners January 20, 2021 10:01

florian-hoenicke requested a review from CatStark January 20, 2021 10:01

JoanFM requested changes Jan 20, 2021

View reviewed changes

fix: cache test

90f3e4f

JoanFM force-pushed the feat-remove-separated-workspace-param branch 2 times, most recently from 208d55c to d73bd0a Compare January 21, 2021 13:41

fix: fix test crud

4bf70b2

JoanFM force-pushed the feat-remove-separated-workspace-param branch 7 times, most recently from 2037d81 to 881d16b Compare January 21, 2021 16:39

fix: consider workspace default None

cb4e38d

JoanFM force-pushed the feat-remove-separated-workspace-param branch from 881d16b to cb4e38d Compare January 21, 2021 16:47

jina-bot added size/XL and removed size/L labels Jan 21, 2021

cristianmtr reviewed Jan 21, 2021

View reviewed changes

florian-hoenicke added 2 commits January 21, 2021 23:59

Merge branch 'master' of https://github.com/jina-ai/jina into feat-re…

8fb1ff6

…move-separated-workspace-param # Conflicts: # jina/parsers/peapods/pea.py

Merge branch 'master' of https://github.com/jina-ai/jina into feat-re…

031a696

…move-separated-workspace-param # Conflicts: # tests/integration/incremental_indexing/test_incremental_indexing.py

jina-bot added size/L and removed size/XL labels Jan 21, 2021

maximilianwerk reviewed Jan 22, 2021

View reviewed changes

JoanFM requested changes Jan 22, 2021

View reviewed changes

jina/executors/__init__.py Outdated Show resolved Hide resolved

fix: update jina/executors/__init__.py

a579aaa

maximilianwerk approved these changes Jan 22, 2021

View reviewed changes

JoanFM approved these changes Jan 22, 2021

View reviewed changes

hanxiao merged commit 75f9569 into master Jan 22, 2021

hanxiao deleted the feat-remove-separated-workspace-param branch January 22, 2021 08:18

JoanFM mentioned this pull request Jan 22, 2021

fix: workspace from dump may not match the one from yaml #1756

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: remove separated workspace param #1722

feat: remove separated workspace param #1722

florian-hoenicke commented Jan 18, 2021 •

edited by JoanFM

JoanFM left a comment

github-actions bot commented Jan 18, 2021 •

edited

codecov bot commented Jan 20, 2021 •

edited

JoanFM left a comment

cristianmtr Jan 21, 2021

florian-hoenicke Jan 21, 2021

cristianmtr Jan 22, 2021

maximilianwerk left a comment

maximilianwerk Jan 22, 2021

maximilianwerk Jan 22, 2021

maximilianwerk Jan 22, 2021

JoanFM Jan 22, 2021

JoanFM Jan 22, 2021

maximilianwerk Jan 22, 2021

JoanFM commented Jan 22, 2021

JoanFM commented Jan 22, 2021

maximilianwerk left a comment

feat: remove separated workspace param #1722

feat: remove separated workspace param #1722

Conversation

florian-hoenicke commented Jan 18, 2021 • edited by JoanFM

Breaking Change #1606

JoanFM left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 18, 2021 • edited

Latency summary

Breakdown

codecov bot commented Jan 20, 2021 • edited

Codecov Report

JoanFM left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximilianwerk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoanFM commented Jan 22, 2021

JoanFM commented Jan 22, 2021

maximilianwerk left a comment

Choose a reason for hiding this comment

florian-hoenicke commented Jan 18, 2021 •

edited by JoanFM

github-actions bot commented Jan 18, 2021 •

edited

codecov bot commented Jan 20, 2021 •

edited