Skip to content
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

feat: optimizer yaml interface #1726

Merged
merged 12 commits into from
Jan 22, 2021
Merged

Conversation

maximilianwerk
Copy link
Member

This will enable using a .yml file for running the optimizer.

  • Added jsonlines parsing to the index_lines and search_lines
    • Please have a look if this is fine. Especially, the change to data_type=DataInputType.AUTO
  • Added simple JAML parsing for any objects which don't need special handling.

Regarding the JAML parsing: I am well aware that the current solution is not in line with how Flows, Executors and Drivers are handled. In case the current solution is not wanted due to broken consistency (or consequences which I have not understood yet), we could also write a simple parser, which is used, whenever no other "special" parser is needed. I would be super happy about better informed input here @hanxiao @JoanFM

@maximilianwerk maximilianwerk requested a review from a team as a code owner January 19, 2021 21:21
@maximilianwerk maximilianwerk marked this pull request as draft January 19, 2021 21:21
@codecov
Copy link

codecov bot commented Jan 19, 2021

Codecov Report

Merging #1726 (1e99cd6) into master (e5e3db8) will decrease coverage by 0.22%.
The diff coverage is 90.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1726      +/-   ##
==========================================
- Coverage   85.55%   85.33%   -0.23%     
==========================================
  Files         134      135       +1     
  Lines        6834     6893      +59     
==========================================
+ Hits         5847     5882      +35     
- Misses        987     1011      +24     
Impacted Files Coverage Δ
jina/enums.py 95.91% <ø> (ø)
jina/optimizers/discovery.py 80.26% <ø> (ø)
jina/peapods/runtimes/jinad/client.py 72.38% <50.00%> (+0.20%) ⬆️
jina/optimizers/flow_runner.py 84.44% <75.00%> (-15.56%) ⬇️
jina/clients/sugary_io.py 96.36% <100.00%> (+0.53%) ⬆️
jina/executors/decorators.py 91.27% <100.00%> (ø)
jina/jaml/parsers/__init__.py 100.00% <100.00%> (+2.27%) ⬆️
jina/jaml/parsers/default/v1.py 100.00% <100.00%> (ø)
jina/logging/logger.py 82.40% <100.00%> (+0.28%) ⬆️
jina/optimizers/parameters.py 100.00% <100.00%> (+10.22%) ⬆️
... and 21 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 ad6306a...1e99cd6. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Jan 19, 2021

Latency summary

Current PR yields:

  • 🐢🐢 index QPS at 1270, delta to last 3 avg.: -27%
  • 😶 query QPS at 33, delta to last 3 avg.: +0%

Breakdown

Version Index QPS Query QPS
current 1270 33
0.9.18 1749 32

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

Copy link
Member

@JoanFM JoanFM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand why current JAML parser does not work?

jina/optimizers/flow_runner.py Outdated Show resolved Hide resolved
jina/flow/__init__.py Outdated Show resolved Hide resolved

class MultiFlowRunner:
class MultiFlowRunner(FlowRunner):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like a lot this pattern

@JoanFM JoanFM changed the title Feat optimizer yaml interface feat: optimizer yaml interface Jan 20, 2021
@maximilianwerk
Copy link
Member Author

I do not understand why current JAML parser does not work?

Because the current JAML parser works ONLY for classes, which inherit from BaseDriver, BaseFlow or BaseExecutor. See this function. We could have another SimpleParseableBaseClass which has a simple parser and works if inherited from.

But on the other hand, that is exactly, what JAMLCompatible is for and thus instead of having a raise NotImplementedError(f'{cls!r} does not implement YAML parser') in the above mentioned function, we could implement a default simple parser. That would be my preferred solution, but before implementing this, I wanted to double check if there are some obvious no-gos.

@JoanFM
Copy link
Member

JoanFM commented Jan 20, 2021

I do not understand why current JAML parser does not work?

Because the current JAML parser works ONLY for classes, which inherit from BaseDriver, BaseFlow or BaseExecutor. See this function. We could have another SimpleParseableBaseClass which has a simple parser and works if inherited from.

But on the other hand, that is exactly, what JAMLCompatible is for and thus instead of having a raise NotImplementedError(f'{cls!r} does not implement YAML parser') in the above mentioned function, we could implement a default simple parser. That would be my preferred solution, but before implementing this, I wanted to double check if there are some obvious no-gos.

I do not understand why current JAML parser does not work?

Because the current JAML parser works ONLY for classes, which inherit from BaseDriver, BaseFlow or BaseExecutor. See this function. We could have another SimpleParseableBaseClass which has a simple parser and works if inherited from.

But on the other hand, that is exactly, what JAMLCompatible is for and thus instead of having a raise NotImplementedError(f'{cls!r} does not implement YAML parser') in the above mentioned function, we could implement a default simple parser. That would be my preferred solution, but before implementing this, I wanted to double check if there are some obvious no-gos.

What I understand now, is that you do not want a Parser?

It seems that this may be a thoughtful design to guarantee that every JAML interface is backed by a CLI one, so maybe we should go for implementing our parser?

@maximilianwerk
Copy link
Member Author

Since we just need a trivial parser for OptunaOptimizer, FlowRunner and OptimizationParsers and the parser might look the same for all three (for now) I wondered, whether I should just implement a simple DefaultParser. And I think, that is what I will do straight away and then we see if it is a good design or not.

Yongxuanzhang and others added 3 commits January 21, 2021 15:42
* feat: added jsonlines loading

* feat: some refactoring for more concistency

* refactor(optimizers): refactor optuna optimizer parameter handling

Co-authored-by: Maximilian Werk <maximilian.werk@jina.ai>
@jina-bot jina-bot added size/L and removed size/M labels Jan 22, 2021
@maximilianwerk maximilianwerk marked this pull request as ready for review January 22, 2021 08:53
@JoanFM
Copy link
Member

JoanFM commented Jan 22, 2021

LGTM!

@maximilianwerk maximilianwerk merged commit 1fffc52 into master Jan 22, 2021
@maximilianwerk maximilianwerk deleted the feat-optimizer-yaml-interface branch January 22, 2021 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants