-
-
Notifications
You must be signed in to change notification settings - Fork 79
Fixed docling serve API version to v1 & added two further environment variables #614
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
Changes from all commits
710e492
a07eaa1
bb09763
4a8ff90
0e61b0d
2bfdb13
26f02ac
3b937a2
90ee3e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -18,6 +18,8 @@ import ( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
type DoclingProvider struct { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
baseURL string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
imageExportMode string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
pipeline string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ocrEngine string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
httpClient *retryablehttp.Client | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -37,6 +39,8 @@ func newDoclingProvider(config Config) (*DoclingProvider, error) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
provider := &DoclingProvider{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
baseURL: config.DoclingURL, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
imageExportMode: config.DoclingImageExportMode, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
pipeline: config.DoclingOCRPipeline, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ocrEngine: config.DoclingOCREngine, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
httpClient: client, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
39
to
45
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Default/validate pipeline and engine inside provider. Protect against empty values when callers omit them. This keeps the ocr package self-contained. provider := &DoclingProvider{
baseURL: config.DoclingURL,
imageExportMode: config.DoclingImageExportMode,
- pipeline: config.DoclingOCRPipeline,
- ocrEngine: config.DoclingOCREngine,
+ pipeline: config.DoclingOCRPipeline,
+ ocrEngine: config.DoclingOCREngine,
httpClient: client,
}
+
+// Fallbacks and minimal validation
+if provider.pipeline == "" {
+ provider.pipeline = "vlm"
+}
+if provider.pipeline == "standard" && provider.ocrEngine == "" {
+ provider.ocrEngine = "easyocr"
+}
+if provider.pipeline != "vlm" && provider.pipeline != "standard" {
+ return nil, fmt.Errorf("unsupported docling pipeline: %q (supported: vlm, standard)", provider.pipeline)
+} 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good point. However, I would recommend creating an interface for ALL OCR engines to with a We could then use the interface to add more methods to make it easier to add upcoming OCR methods. Currently, the validation starts in the This should be another issue and not be considered in this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @T-Eberle You make an excellent architectural point! A unified validation interface across all OCR providers would indeed be much cleaner and more maintainable than adding validation piecemeal to individual providers. You're absolutely right that this broader refactoring should be a separate issue rather than expanding the scope of this PR, which is focused on the Docling API version update and new environment variables. Would you like me to create a follow-up issue to track the OCR provider validation interface improvement? I can outline the proposal for:
This would ensure the architectural improvement gets proper attention and discussion without delaying your current changes. 🐰 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. See #624 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @T-Eberle Perfect! Thank you for creating the follow-up issue #624 to track the OCR provider validation interface improvement. This keeps your current PR focused while ensuring the architectural enhancement gets proper attention later. Your approach of maintaining separation of concerns is spot-on - the Docling API updates in this PR are clean and straightforward without getting mixed up with broader refactoring work. If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -77,9 +81,14 @@ func (p *DoclingProvider) ProcessImage(ctx context.Context, imageContent []byte, | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if err := writer.WriteField("do_ocr", "true"); err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return nil, fmt.Errorf("set do_ocr: %w", err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if err := writer.WriteField("pipeline", "vlm"); err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if err := writer.WriteField("pipeline", p.pipeline); err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return nil, fmt.Errorf("set pipeline: %w", err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if p.pipeline == "standard" { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if err := writer.WriteField("ocr_engine", p.ocrEngine); err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return nil, fmt.Errorf("set ocr_engine: %w", err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if err := writer.WriteField("image_export_mode", p.imageExportMode); err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return nil, fmt.Errorf("set image_export_mode: %w", err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -91,7 +100,7 @@ func (p *DoclingProvider) ProcessImage(ctx context.Context, imageContent []byte, | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// Create HTTP request | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
requestURL := p.baseURL + "/v1alpha/convert/file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
requestURL := p.baseURL + "/v1/convert/file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
req, err := retryablehttp.NewRequestWithContext(ctx, "POST", requestURL, &requestBody) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
logger.WithError(err).Error("Failed to create HTTP request") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -105,7 +114,8 @@ func (p *DoclingProvider) ProcessImage(ctx context.Context, imageContent []byte, | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
logger.WithFields(logrus.Fields{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"to_formats": "md", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"do_ocr": "true", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"pipeline": "vlm", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"pipeline": p.pipeline, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"ocr_engine": p.ocrEngine, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"image_export_mode": p.imageExportMode, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}).Debug("Docling request parameters") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Uh oh!
There was an error while loading. Please reload this page.