Navigation Menu

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

Web api #222

Merged
merged 87 commits into from Mar 3, 2023
Merged

Web api #222

merged 87 commits into from Mar 3, 2023

Conversation

tdoan2010
Copy link
Contributor

This is my first attempt for the Web API documentation. Comments are welcome.

@tdoan2010 tdoan2010 self-assigned this Aug 19, 2022
@tdoan2010 tdoan2010 linked an issue Aug 19, 2022 that may be closed by this pull request
lena-hinrichsen added a commit that referenced this pull request Aug 29, 2022
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.

Mostly formulations, but some issues too.

web_api.md Outdated Show resolved Hide resolved
web_api.md Outdated Show resolved Hide resolved
web_api.md Outdated Show resolved Hide resolved
web_api.md Outdated Show resolved Hide resolved
web_api.md Outdated Show resolved Hide resolved
web_api.md Outdated Show resolved Hide resolved
web_api.md Outdated Show resolved Hide resolved
web_api.md Outdated Show resolved Hide resolved
web_api.md Outdated Show resolved Hide resolved
web_api.md Outdated Show resolved Hide resolved
@tdoan2010
Copy link
Contributor Author

What if the server in question does not use queues, but directly schedules local CLI calls or distributed Processor REST calls?

That's a valid argument for having the callback feature. I will update the spec with this.

@tdoan2010
Copy link
Contributor Author

@bertsky could you resolve those conversations which are "done"? 😃 I know that it's still missing a section to describe the standalone REST processors, but I'll add it later, since I don't want to make this PR bigger (it's already big... 😅 ).

@bertsky
Copy link
Collaborator

bertsky commented Jan 26, 2023

@bertsky could you resolve those conversations which are "done"? I know that it's still missing a section to describe the standalone REST processors, but I'll add it later, since I don't want to make this PR bigger (it's already big... ).

Done. Three points remain open for me, hopefully we can still work these through.

@tdoan2010
Copy link
Contributor Author

Done. Three points remain open for me, hopefully we can still work these through.

Sorry, but I only found 2 (the arrow direction and the sync between producers & consumers). What do I miss? 😅

@bertsky
Copy link
Collaborator

bertsky commented Jan 26, 2023

What do I miss?

the one about an optional POST /workspace for METS URLs – since it's not in the swagger defs yet, and no mention of it being OPTIONAL in the markdown page, I assumed it's still open

@tdoan2010
Copy link
Contributor Author

the one about an optional POST /workspace for METS URLs – since it's not in the swagger defs yet, and no mention of it being OPTIONAL in the markdown page, I assumed it's still open

I will update the openapi.yml file after this PR is merged. Then, that endpoint will be there and marked as OPTIONAL, as we discussed.

web_api.md Outdated Show resolved Hide resolved
web_api.md Outdated Show resolved Hide resolved
web_api.md Outdated Show resolved Hide resolved
web_api.md Outdated Show resolved Hide resolved
web_api.md Show resolved Hide resolved
@bertsky
Copy link
Collaborator

bertsky commented Feb 10, 2023

Regarding https://github.com/OCR-D/spec/pull/222/files/ad6fe9eb018a40b823161331876f4afaad1827b3..3b2dee9d7c043f61ce04d3369b941f20ec78af2f – I do think sub-grouping is possible. It's been done in core in various places (e.g. ocrd-tool sub-group), too.

@tdoan2010
Copy link
Contributor Author

Regarding https://github.com/OCR-D/spec/pull/222/files/ad6fe9eb018a40b823161331876f4afaad1827b3..3b2dee9d7c043f61ce04d3369b941f20ec78af2f – I do think sub-grouping is possible. It's been done in core in various places (e.g. ocrd-tool sub-group), too.

@kba said that it's really complicated to implement sub-command in the OCR-D Click Wrapper. So, better use option.

@bertsky
Copy link
Collaborator

bertsky commented Feb 10, 2023

@kba said that it's really complicated to implement sub-command in the OCR-D Click Wrapper. So, better use option.

I don't see it. It's just commands inside of groups inside of commands. A good example is ocrd-tool group. Implementation is easy. I could understand if the argument is that it's more complicated to document (esp. in the CLI). But then again, multiple related, interdependent options are also not easy not explain...

@kba
Copy link
Member

kba commented Feb 13, 2023

I don't see it. It's just commands inside of groups inside of commands. A good example is ocrd-tool group. Implementation is easy. I could understand if the argument is that it's more complicated to document (esp. in the CLI). But then again, multiple related, interdependent options are also not easy not explain...

The problem is that we generate the click interface programmatically, so that processor developers only need

@click.command()                                                   
@ocrd_cli_options                                                  
def cli(*args, **kwargs):                                          
    return ocrd_cli_wrap_processor(MyProcessor, *args, **kwargs)

Now, we either require processors to change that to

@click.group()
@ocrd_cli_options
def cli(...):
   return ocrd_cli_wrap_processor(...)

@cli.command('server')
def worker_cli(...):
  return ocrd_cli_worker_subcommand_wrap(MyProcessor)

or find a way to wrap this as a single annotation. It's possible but tricky.

Whereas, for the ocrd command, it's trivial to create subcommands because they use the standard click-way of creating CLI.

@kba kba merged commit ae533ed into master Mar 3, 2023
@kba kba deleted the web-api branch March 3, 2023 16:05
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.

Add Web API Spec Docu
4 participants