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

Factor setup #31

Merged
merged 4 commits into from
Apr 20, 2022
Merged

Factor setup #31

merged 4 commits into from
Apr 20, 2022

Conversation

bertsky
Copy link
Contributor

@bertsky bertsky commented Jun 9, 2021

In preparation of upcoming API changes – notably automatic page-level parallelism and page-level failover – this factors out the model loading from process() and from the constructor into a dedicated, conventional setup() (which will be required/used by the Processor base class in core soon, at which point there will be no need to call it from the inherited constructor).

Note that this also already enables running sbb-binarize in the workflow server prototype.

The second change is necessary if you have multiple TF sessions in your server: each must store its session and reload it just prior to running the graph.

Copy link
Collaborator

@kba kba left a comment

Choose a reason for hiding this comment

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

The setup and docstring changes as well as the {,self}.binarizer fix are clear improvements. For the TF session management changes, @vahidrezanezhad or @mikegerber know better about any potential pitfalls. To me, it looks good.

@cneud cneud requested a review from vahidrezanezhad April 19, 2022 15:36
Copy link
Member

@cneud cneud left a comment

Choose a reason for hiding this comment

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

LGTM, thx @bertsky!

(@vahidrezanezhad can you plz also check and sign off?)

@vahidrezanezhad vahidrezanezhad merged commit e157526 into qurator-spk:master Apr 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants