Skip to content

[Sidecar] Add ability to remove one or all references#193

Merged
cguedes merged 8 commits into
mainfrom
191-sidecar-add-ability-to-remove-one-or-all-references
Jun 26, 2023
Merged

[Sidecar] Add ability to remove one or all references#193
cguedes merged 8 commits into
mainfrom
191-sidecar-add-ability-to-remove-one-or-all-references

Conversation

@gjreda

@gjreda gjreda commented Jun 23, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator

fixes #191

# Example:
$ poetry run python main.py delete --source_filenames grobid-fails.pdf "Machine Learning at Scale.pdf" | jq

# Response:
{
  "status": "ok",
  "message": ""
}

# Another Example (error):
$ poetry run python main.py delete --source_filenames file-does-not-exist.pdf | jq

# Error Response:
{
  "status": "error",
  "message": "Unable to delete file-does-not-exist.pdf: not found in storage"
}

@codecov

codecov Bot commented Jun 23, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #193 (6d31caf) into main (30cd1b5) will increase coverage by 0.04%.
The diff coverage is 73.46%.

@@            Coverage Diff             @@
##             main     #193      +/-   ##
==========================================
+ Coverage   66.91%   66.96%   +0.04%     
==========================================
  Files          87       88       +1     
  Lines        4498     4541      +43     
  Branches      331      331              
==========================================
+ Hits         3010     3041      +31     
- Misses       1471     1483      +12     
  Partials       17       17
Impacted Files Coverage Δ
python/main.py 0.00% <0.00%> (ø)
python/sidecar/cli.py 0.00% <0.00%> (ø)
python/sidecar/storage.py 90.66% <80.76%> (-5.34%) ⬇️
python/sidecar/ingest.py 90.82% <100.00%> (-0.14%) ⬇️
python/sidecar/settings.py 100.00% <100.00%> (ø)
python/sidecar/typing.py 96.96% <100.00%> (+0.30%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@gjreda gjreda marked this pull request as ready for review June 23, 2023 17:32
@gjreda gjreda requested review from cguedes and sehyod June 23, 2023 17:32
sehyod
sehyod previously approved these changes Jun 26, 2023
View reviewed changes
cguedes
cguedes requested changes Jun 26, 2023
View reviewed changes

@cguedes cguedes left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Fix the CliCommands response payload.

Also, I would prefer to have an extra argument (ex: --all) to remove all files and another argument for selected files (--source_filenames ...).

Comment thread src/api/types.ts Outdated
@gjreda gjreda requested review from cguedes and sehyod June 26, 2023 15:55
cguedes
cguedes requested changes Jun 26, 2023
View reviewed changes

@cguedes cguedes left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The current implementation only updates the references metadata (references.json) and don't remove any other files (pdf, .grobid, ...).

This behaviour (i.e. the PDF inside uploads) makes the ingestion a new PDF (i.e. a different PDF) to also ingest previously deleted PDFs.

@gjreda

gjreda commented Jun 26, 2023

Copy link
Copy Markdown
Collaborator Author

I think it makes sense for the front-end to delete the actual PDF from the filesystem, since the front-end is currently what writes the PDF to the uploads directory.

I agree that in the future this should probably all be owned by the backend though, particularly in a world where the backend is some set of web APIs.

@cguedes cguedes merged commit 7f9cc08 into main Jun 26, 2023
@cguedes cguedes deleted the 191-sidecar-add-ability-to-remove-one-or-all-references branch June 26, 2023 16:32
@cguedes cguedes mentioned this pull request Jun 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cguedes cguedes cguedes approved these changes
@sehyod sehyod Awaiting requested review from sehyod

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Sidecar] Add ability to remove one or all references

3 participants

@gjreda @cguedes @sehyod