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

Cloud func and misc cleanup #26

Merged
merged 6 commits into from
Apr 18, 2024
Merged

Cloud func and misc cleanup #26

merged 6 commits into from
Apr 18, 2024

Conversation

vincerubinetti
Copy link
Collaborator

@vincerubinetti vincerubinetti commented Apr 13, 2024

Note: the deploy preview here won't work since it will be hitting the live cloud functions which are not updated with the changes in this PR yet. If you want to test just the frontend on the Netlify preview, put ?mock=true at the end of the url and it will use fake data instead of a real api.

  • convert cloud funcs and consuming frontend api funcs to use json-encoded body params instead of url params
  • make some frontend types more specific
  • fix button component unique key warning
  • remove vestigial "icon" className in a few places
  • display summary of inputs on results page
  • add per-species example genes
  • make cloud functions use POST and request body for passing params
  • return appropriate status codes and error messages from cloud funcs
  • move documentation of cloud funcs to separate readme for readability (headings to split up/organize content, code blocks for syntax highlighting, etc). do a bit of rewriting of comments for clarity.
  • rename "run_pipeline" to "ml" for consistency

Copy link

netlify bot commented Apr 13, 2024

Deploy Preview for gene-plexus ready!

Name Link
🔨 Latest commit ca6de01
🔍 Latest deploy log https://app.netlify.com/sites/gene-plexus/deploys/661d5b923addcc00085ea403
😎 Deploy Preview https://deploy-preview-26--gene-plexus.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@vincerubinetti
Copy link
Collaborator Author

I forgot to open this PR yesterday. I wanted to open it today so I didn't forget and to get it off my mind, but I'd recommend waiting until Monday to review.

@ChristopherMancuso You can run Faisal's run_local.sh script to test the entire stack out locally.

@ChristopherMancuso
Copy link
Contributor

I was able to look at this with run_local.sh and look over the function commit updates.

It is nice to have pulled the info not he function input and outputs out into a readme. For that though, there is only a README.md in the ml function folder and noting for the id conversion. And in the ML README there are two sections called gpz_convert_ids. If Im following what is going on, should the top part in the ML README.md be put in a READ me in the id_convert folder?

@vincerubinetti
Copy link
Collaborator Author

I mistakenly had that in the ml folder, see the last commit where I move it up to the folder above it.

Please also make sure the notes I wrote are correct. There are probably some gaps that could be filled, like a top level comment above df_probs and df_sim.

@ChristopherMancuso
Copy link
Contributor

I just checked over that readme file and committed some updates.

Also, I did try training in one species and testing in another and that didn't work. Probably not for this PR to worry about, right?

@vincerubinetti
Copy link
Collaborator Author

Also, I did try training in one species and testing in another and that didn't work

How do you mean? Did you do this through the API directly? The frontend keeps them the same species for now, as we discussed.

@ChristopherMancuso
Copy link
Contributor

I did it on the front end in run_local.sh. I had forgot you said the API was restricting to the species being the same in each of the buttons for now.

Copy link
Contributor

@ChristopherMancuso ChristopherMancuso left a comment

Choose a reason for hiding this comment

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

I think it all looks good to me for this PR.

@vincerubinetti
Copy link
Collaborator Author

I added one more commit, please check it. I included the inputs for /ml back in the output. This way, the results.json that we get back from the server is a "complete package", and when a user saves it as json and reuploads it, it will populate the "inputs" section in the UI and remind them what the job parameters were.

Copy link
Collaborator

@falquaddoomi falquaddoomi left a comment

Choose a reason for hiding this comment

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

Nice work!

On a side note, I'm happy to see you found and updated the entrypoint.sh script to use the new entrypoint for ml.

@vincerubinetti vincerubinetti merged commit 6ea8441 into main Apr 18, 2024
3 checks passed
@vincerubinetti vincerubinetti deleted the cloud-cleanup branch April 18, 2024 20: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.

3 participants