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

fix: workaround for error in getByJobTypeAndJobIdAndRunId #102

Merged
merged 24 commits into from
Jun 6, 2024

Conversation

bodom0015
Copy link
Member

@bodom0015 bodom0015 commented May 16, 2024

Problem

Frontend has trouble fetching job results after adding Kubernetes job support

Approach

  • fix: temporary workaround to use getByJobTypeAndJobId instead - we can simply assume a single run for now

How to Test

Test this with moleculemaker/mmli-backend#43

Currently deployed on prod: https://chemscraper.platform.moleculemaker.org

  1. Navigate to https://chemscraper.platform.moleculemaker.org/configuration
  2. Submit a real (known-good) PDF for analysis
  3. Wait for job to complete
    • You should eventually see the results load and display

@KastanDay
Copy link
Contributor

KastanDay commented May 16, 2024

In testing, after uploading a file the button "Analyze with Chemscraper" remains disabled, so I can't submit the job. Same with the example pdf.

The network tab shows the Upload responds with 200.

Request URL: https://mmli.fastapi.staging.mmli1.ncsa.illinois.edu/chemscraper/upload?job_id=
Request Method: POST
Status Code: 200 OK

CleanShot 2024-05-16 at 15 25 30

@bodom0015
Copy link
Member Author

bodom0015 commented May 17, 2024

Thank you, @KastanDay! I think you fixed this issue in the base branch, so it should be fixed here as well 👍

Is it ok that we are merging this into design-audit-feedback-round-2? Or should I put this on main instead and have you pull into your branch?

Do you have any preference here?

@bodom0015 bodom0015 changed the base branch from design-audit-feedback-round-2 to main May 28, 2024 21:58
@KastanDay
Copy link
Contributor

@bodom0015 No preference! Go for main, then it's nice and independent.

Copy link
Contributor

@KastanDay KastanDay left a comment

Choose a reason for hiding this comment

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

I tested with a random chemistry PDF, it worked great. Approved, recommend merge.

@KastanDay
Copy link
Contributor

I tested again today with both the Example PDF and a random chem PDF, still working great. Merging.

@KastanDay KastanDay merged commit 2f6f84e into main Jun 6, 2024
1 check passed
@KastanDay KastanDay deleted the bugfix/fix-file-paths-and-result-fetch branch June 6, 2024 18:04
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.

2 participants