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.

Sign up for GitHub

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

Jump to bottom

chore(#994): simplify server start #1014

Merged
merged 5 commits into from
Jan 24, 2022
Merged

chore(#994): simplify server start #1014

merged 5 commits into from
Jan 24, 2022

Conversation

frascuchon
Copy link
Member

@frascuchon frascuchon commented Jan 21, 2022 •
edited
Loading

This PR changes how to launch rubrix server to:

python -m rubrix

The old command (python -m rubrix.server) is also available

Also, include setup info about uvicorn setup. Basically, you can launch rubrix server using uvicorn:

uvicorn rubrix:app

@frascuchon frascuchon changed the title chore: simplify server start chore(#994): simplify server start Jan 21, 2022
@codecov
Copy link

codecov bot commented Jan 21, 2022 •
edited
Loading

Codecov Report

Merging #1014 (a49e81e) into master (15b3eb4) will decrease coverage by 0.22%.
The diff coverage is 23.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1014      +/-   ##
==========================================
- Coverage   95.36%   95.14%   -0.23%     
==========================================
  Files         110      111       +1     
  Lines        4248     4260      +12     
==========================================
+ Hits         4051     4053       +2     
- Misses        197      207      +10
Flag Coverage Δ
pytest 95.14% <23.07%> (-0.23%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/rubrix/__main__.py 0.00% <0.00%> (ø)
src/rubrix/server/__main__.py 0.00% <0.00%> (ø)
src/rubrix/__init__.py 85.10% <37.50%> (-9.90%) ⬇️

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 15b3eb4...a49e81e. Read the comment docs.

dcfidalgo
dcfidalgo approved these changes Jan 21, 2022
View reviewed changes
Copy link
Contributor

@dcfidalgo dcfidalgo left a comment

Choose a reason for hiding this comment

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

💯 Just a few inline suggestions. also, the setup&installation docs will be updated soon due to #994.

docs/getting_started/setup&amp;installation.rst Outdated Show resolved Hide resolved
src/rubrix/__init__.py Show resolved Hide resolved
@dcfidalgo
Copy link
Contributor

@frascuchon ah, I forgot, if we want to deprecate the python -m rubrix.server command in the future, I would add a FutureWarning.

@dvsrepo
Copy link
Member

dvsrepo commented Jan 21, 2022 •
edited
Loading

One additional comment, which is a feedback we get often. I don't know where is the best place but I would like to have a very clear message when some starts the server and the Elasticsearch endpoint is not available/running, etc, I don't know if we check that at some point but I recall the message it's a standard trace. We should be very informative at this point, explaining:

  1. We checked the elastic search endpoint at {ES_URL} and is not responding/available..
  2. Please make sure your Elasticsearch is running, or (check the user roles config) <- whatever message it makes sense.

Ideally this should be applicable to other scenarios when the ES instance is not responding --> including a trace in the server process.

I think this is key to help our users troubleshoot the issues with Elastic which is frequently the only bottleneck for getting started.

I understand this should go on another issue/PR

@frascuchon
Copy link
Member Author

Member

Yes, i prefer include in a separate PR. Could you create a issue to track the change @dvsrepo ?

@frascuchon
Copy link
Member Author

@frascuchon ah, I forgot, if we want to deprecate the python -m rubrix.server command in the future, I would add a FutureWarning.

Done

@frascuchon
Update docs/getting_started/setup&installation.rst
a49e81e 
Co-authored-by: David Fidalgo <david@recogn.ai>
@frascuchon frascuchon merged commit 7667ae8 into master Jan 24, 2022
@frascuchon frascuchon deleted the refactor/simplify-server-start branch January 24, 2022 14:25
dvsrepo added a commit that referenced this pull request Jan 25, 2022
@dvsrepo
Merge branch 'master' of https://github.com/recognai/rubrix
8f89dda 
* 'master' of https://github.com/recognai/rubrix: (28 commits)
  chore(#994): simplify server start (#1014)
  refactor(#945): elasticsearch index configuration per task (#1018)
  docs(#980): Add majorityvote example to weak supervision guide (#1004)
  fix(#1015): manage emojis in Token Classification records (#1016)
  fix(#1010): fix WeakLabels when not providing rules (#1011)
  fix(Breadcrumbs): Copy text function (#1003)
  fix(statics): handle 404 errors for static files (#1006)
  refactor: Rename 'exclude_missing_annotation' argument for WeakLabels.annotation (#1005)
  docs: update video for spacy ner tutorial (#1000)
  fix(#977) Remove redirection when accessing login (#996)
  feat(#951): new uncovered_by_rules records filter (#991)
  feat(#735): add warning when agent but no prediction/annotation is provided (#987)
  fix(#800): limit number of metadata fields (#993)
  docs: improve annotate records section in web app reference (#985)
  fix(#844) Keep header layout when errors occurred in dataset view (#992)
  fix(#974): display the dropdown in the last record of the scroll (#986)
  refactor: compute common aggregations one by one (#990)
  feat(#953): add additional metrics to `LabelModel.score` method (#979)
  refactor(rules): using labeling rules management with vuex store (#878)
  feat(#955): add default for `rules` in WeakLabels (#976)
  ...
frascuchon added a commit that referenced this pull request Feb 2, 2022
@frascuchon
chore(#994): simplify server start (#1014)
504c3f6 
* refactor: move server launch to main module

* docs: update setup section

* revert: remove old __main__ file

* chore: include FutureWarning

* Update docs/getting_started/setup&installation.rst

Co-authored-by: David Fidalgo <david@recogn.ai>

Co-authored-by: David Fidalgo <david@recogn.ai>
(cherry picked from commit 7667ae8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcfidalgo dcfidalgo dcfidalgo approved these changes

@dvsrepo dvsrepo Awaiting requested review from dvsrepo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@frascuchon @dcfidalgo @dvsrepo