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

Update models docs to core 2.22.0 #196

Merged
merged 21 commits into from
Jan 26, 2021
Merged

Update models docs to core 2.22.0 #196

merged 21 commits into from
Jan 26, 2021

Conversation

kba
Copy link
Member

@kba kba commented Jan 19, 2021

No description provided.

@kba kba requested review from bertsky and EEngl52 January 19, 2021 18:13
Copy link
Collaborator

@bertsky bertsky left a comment

Choose a reason for hiding this comment

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

Should we explicitly mention presets as a special kind of resource?

And should we give a perspective on practical resource management with ocrd_all native and ocrd/all containers here? (Just asking, really.)

site/en/models.md Outdated Show resolved Hide resolved
site/en/models.md Outdated Show resolved Hide resolved
site/en/models.md Outdated Show resolved Hide resolved
site/en/models.md Outdated Show resolved Hide resolved
site/en/models.md Outdated Show resolved Hide resolved
site/en/models.md Outdated Show resolved Hide resolved
site/en/models.md Outdated Show resolved Hide resolved
site/en/models.md Outdated Show resolved Hide resolved
site/en/models.md Show resolved Hide resolved
Copy link
Collaborator

@EEngl52 EEngl52 left a comment

Choose a reason for hiding this comment

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

looks good to me. We should also adjust the example calls in the workflow guide

site/en/models.md Outdated Show resolved Hide resolved
site/en/models.md Show resolved Hide resolved
kba and others added 10 commits January 20, 2021 11:47
Co-authored-by: Robert Sachunsky <38561704+bertsky@users.noreply.github.com>
Co-authored-by: Elisabeth Engl <53007946+EEngl52@users.noreply.github.com>
Co-authored-by: Robert Sachunsky <38561704+bertsky@users.noreply.github.com>
Co-authored-by: Robert Sachunsky <38561704+bertsky@users.noreply.github.com>
Co-authored-by: Robert Sachunsky <38561704+bertsky@users.noreply.github.com>
Co-authored-by: Robert Sachunsky <38561704+bertsky@users.noreply.github.com>
Co-authored-by: Robert Sachunsky <38561704+bertsky@users.noreply.github.com>
Co-authored-by: Robert Sachunsky <38561704+bertsky@users.noreply.github.com>
Co-authored-by: Robert Sachunsky <38561704+bertsky@users.noreply.github.com>
Co-authored-by: Robert Sachunsky <38561704+bertsky@users.noreply.github.com>
@kba
Copy link
Member Author

kba commented Jan 20, 2021

Should we explicitly mention presets as a special kind of resource?

Good point, I haven't been considering parameter presets, I'll have to make sure that it is properly covered and then document it. So I'll make another pre-release and do a proper release once I am sure we have this covered.

And should we give a perspective on practical resource management with ocrd_all native and ocrd/all containers here? (Just asking, really.)

Yes! One reason for resmgr was to make it easier to mount models into docker and we should document that this is now possible.

@kba
Copy link
Member Author

kba commented Jan 21, 2021

One reason for resmgr was to make it easier to mount models into docker and we should document that this is now possible.

d886e68 @bertsky I don't have docker available right now, so I haven't tested this. Can you check please whether this works? In particular, what is $HOME in ocrd_all? Since we advise to use the docker run --user flag, I am fairly certain, it should be / but not entirely sure. Thanks!

@bertsky
Copy link
Collaborator

bertsky commented Jan 21, 2021

d886e68

Forgot to push?

I don't have docker available right now, so I haven't tested this. Can you check please whether this works? In particular, what is $HOME in ocrd_all?

HOME expands to /root in ocrd/all.

Since we advise to use the docker run --user flag, I am fairly certain, it should be / but not entirely sure. Thanks!

Yes, with -u HOME becomes /.

@kba
Copy link
Member Author

kba commented Jan 21, 2021

Forgot to push?

Yup, sry 😊 pushed it now.

Yes, with -u HOME becomes /.

Thanks for checking.

Copy link
Collaborator

@bertsky bertsky left a comment

Choose a reason for hiding this comment

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

There were various inaccuracies in the commands, but more importantly, I don't think we should actually recommend that 2-step approach. You would still have to natively install core independent of Docker, which defeats the use-case for virtualization in the first place. (And in the future, we even want to aggregate the database from submodules on the fly, so a full-blown native ocrd_all installation would be necessary to download resources on the host.)

Instead, this alternative formulation just explains how to mount a persistent host directory for the models, and use Docker to download models into it whenever needed.

site/en/models.md Outdated Show resolved Hide resolved
site/en/models.md Outdated Show resolved Hide resolved
@kba
Copy link
Member Author

kba commented Jan 22, 2021

Instead, this alternative formulation just explains how to mount a persistent host directory for the models, and use Docker to download models into it whenever needed.

You're right, I didn't consider how you'd need a full deployment on the host to download the models outside of docker.

And IIRC for that very reason we also wanted to make --location virtualenv the default in the Docker build...

This works on the top-level venv but what about the sub-venvs? They have their own $VIRTUAL_ENV and hence the ocrd resmgr in the sub-venvs will not see the model installed to top-level $VIRTUAL_ENV/share/ocrd-resources :( Correct me if I'm wrong but I think we do need either XDG or re-introduce an absolute path like /usr/share/local/ocrd-resources ...

site/en/models.md Outdated Show resolved Hide resolved
site/en/models.md Outdated Show resolved Hide resolved
site/en/models.md Outdated Show resolved Hide resolved
site/en/models.md Outdated Show resolved Hide resolved
kba and others added 4 commits January 22, 2021 17:27
Co-authored-by: Robert Sachunsky <38561704+bertsky@users.noreply.github.com>
Co-authored-by: Robert Sachunsky <38561704+bertsky@users.noreply.github.com>
Co-authored-by: Robert Sachunsky <38561704+bertsky@users.noreply.github.com>
Co-authored-by: Robert Sachunsky <38561704+bertsky@users.noreply.github.com>
kba and others added 2 commits January 22, 2021 17:32
Co-authored-by: Robert Sachunsky <38561704+bertsky@users.noreply.github.com>
site/en/models.md Outdated Show resolved Hide resolved
Co-authored-by: Robert Sachunsky <38561704+bertsky@users.noreply.github.com>
@kba kba merged commit 3d25337 into master Jan 26, 2021
@kba kba deleted the update-models-2.22.0 branch January 26, 2021 12:11
@bertsky
Copy link
Collaborator

bertsky commented Jan 26, 2021

Sorry, I was not aware the markdown rendering is different on ocr-d.de. It did not take my four-space prefix as command syntax, we must change to code blocks (inside list items!) again...

@kba
Copy link
Member Author

kba commented Jan 26, 2021

I was afraid so but didn't check. I'll prepare a fix.

@bertsky
Copy link
Collaborator

bertsky commented Jan 26, 2021

Also, we still need to explain that now processors will automatically download resources they need. (This is a bit surprising given all the ocrd resmgr facilities. So it should be made clear.)

@kba
Copy link
Member Author

kba commented Jan 26, 2021

Don't we mention "downloaded on-demand"?

@bertsky
Copy link
Collaborator

bertsky commented Jan 26, 2021

Don't we mention "downloaded on-demand"?

Now that you say it: yes, once, in a comment of a command for one processors' resources 😄

@kba
Copy link
Member Author

kba commented Jan 26, 2021

Now that you say it: yes, once, in a comment of a command for one processors' resources 😄

that's all a true hacker needs : Jest aside, I added a section on that in #197

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.

3 participants