staging: run recipes from firebase #179

rugeli · 2023-07-13T19:14:42Z

Problem

What is the problem this work solves, including
closes #167

Solution

This is a staging branch for run recipes from firebase.

features in this branch that need to be reviewed:

a new DBUploader class for data conversion and uploading recipes into modules

already reviewed:

firebase credential input
AWSHandler for storing simularium results to S3
save metadata to firebase
open a new tab for simularium result
handlers are initiated using the DATABASE_IDS enum

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)

Steps to Verify:

Have firebase project setup and your personal credentials ready
Upload a recipe to your firebase: upload -r examples/recipes/v2/gradients.json
Pack the remote recipe you uploaded: pack -r firebase:recipes/gradients_v_default -c examples/packing-configs/run.json
check up the result on Simularium, AWS S3 and Firebase

Keyfiles (delete if not relevant):

DBRecipeHandler.py
autopack/__init__.py

* formatting

…m/mesoscope/cellpack into feature/run-recipes-from-firebase

github-actions · 2023-07-13T23:02:57Z

Packing analysis report

Analysis for packing results located at cellpack/tests/outputs/test_spheres/spheresSST

Ingredient name	Encapsulating radius	Average number packed
ext_A	25	236.0

Packing image

Distance analysis

Expected minimum distance: 50.00
Actual minimum distance: 50.01

Ingredient key	Pairwise distance distribution
ext_A

codecov-commenter · 2023-08-02T23:48:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (37b661a) 98.52% compared to head (93787fc) 98.63%.
Report is 16 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #179      +/-   ##
==========================================
+ Coverage   98.52%   98.63%   +0.10%     
==========================================
  Files          16       18       +2     
  Lines         476      511      +35     
==========================================
+ Hits          469      504      +35     
  Misses          7        7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ture/run-recipes-from-firebase

* turn off resolving inheritance while uploading * able to upload recipes having "inherit" key * get download and pack to work, refactors needed * refactors

…ture/run-recipes-from-firebase

…m/mesoscope/cellpack into feature/run-recipes-from-firebase

…ture/run-recipes-from-firebase

* refactor AWS and firebase handler * databases initiation handling

rugeli · 2023-11-29T22:09:34Z

In this PR, I’d appreciate your focus on reviewing three main parts:

autopack/init.py/load_file - where the firebase recipe data is detected and accessed
recipe_loader.py/_read - this is where the firebase data is read and load
DBRecipeHanlder.py/DBRecipeLoader - where the db data to local data format conversion happens

Also, sorry for mixing the already-reviewed features with the awaiting-review features in one PR. I’ll manage my PR flow better moving forward to make it easier for everyone to review.

mogres

Can you add instructions to create firebase credentials somewhere? These could be a page in the wiki that can be linked elsewhere.

The updated README is in the upload local recipes to s3PR - #189, you can view the instructions here: https://github.com/mesoscope/cellpack/pull/189/files

So after some discussion and having addressed the changes, the recommended PR merge order should be: #191 - #189 - (WIP) run recipes with "inherit" key - #179, feel free to review the first two when you have time. I'll convert this PR to a draft for now. Sorry for the confusion!

Maybe we could also add the link to create firebase credentials in the prompt to provide the credentials path?

mogres · 2023-09-14T23:36:01Z

cellpack/autopack/loaders/utils.py

+        return json.load(file_name)
+
+
+def write_json_file(path, data):


This will overwrite existing files at the path. Would it make sense to add an option for checking this?

I just found out that we have checks for existing keys and contents(like user and firebase creds) before calling this function. So it should be safe to leave this method in its original form.

mogres · 2023-12-12T19:54:40Z

cellpack/autopack/loaders/recipe_loader.py

+                        and "all_partners" in obj["partners"]
+                        and not obj["partners"]["all_partners"]
+                    )
+                    else obj.get("partners", [])


for recipes downloaded from a remote db, do we expect this to work if obj["partners"]["all_partners"] is not empty?

Thank you for asking this! The if statement here is to determine whether the recipes retrieved from firebase have partners. During the upload process, the key "all_partners" is added to "partners" of each obj no matter there is partner or not.

The structure of the partners in Firebase is as follows:

for recipes without partners: obj[partners] == {'all_partners': []}

for recipes with partners: obj[partners] == {'all_partners': [<cellpack.autopack.interface_objects.partners.Partner object at xyz>]}

mogres · 2023-12-12T19:58:50Z

cellpack/autopack/loaders/recipe_loader.py

-        if "gradients" in recipe_data:
+        # gradients in firebase recipes are already stored as a list of dicts
+        if "gradients" in recipe_data and not isinstance(
+            recipe_data["gradients"], list


Do we need to initialize GradientData instances from the existing list of dictionaries on firebase? i.e. loop over the list (instead of a dict) and instantiate?

Just like with Partners, the cellpack system organizes gradients into a list during the upload process. Therefore, we can skip processing gradients in firebase recipes.

mogres · 2023-12-12T20:08:40Z

cellpack/tests/test_data_doc.py

+def test_is_nested_list():
+    assert DataDoc.is_nested_list([]) is False
+    assert DataDoc.is_nested_list([[], []]) is True
+    assert DataDoc.is_nested_list([[1, 2], [3, 4]]) is True


Should DataDoc.is_nested_list([1, [1, 2]]) be True or False?

Good thought! Currently it returns False, though it should return True. While we don't typically encounter such nested lists like [1, [1, 2]] in our recipe data, but you are right that our method should be robust enough to handle all types of nested lists. I just modified the function to run a loop, should be good this time.

.gitignore

mogres · 2023-12-12T20:22:40Z

Looks good overall! I was able to upload and run the gradient recipe remotely. Thanks for adding detailed tests and documentation :)

rugeli · 2023-12-12T21:52:16Z

Can you add instructions to create firebase credentials somewhere? These could be a page in the wiki that can be linked elsewhere.

The updated README is in the upload local recipes to s3PR - #189, you can view the instructions here: https://github.com/mesoscope/cellpack/pull/189/files

So after some discussion and having addressed the changes, the recommended PR merge order should be: #191 - #189 - (WIP) run recipes with "inherit" key - #179, feel free to review the first two when you have time. I'll convert this PR to a draft for now. Sorry for the confusion!

Maybe we could also add the link to create firebase credentials in the prompt to provide the credentials path?

Good idea! We are going to refactor the the authentication methods to integrate firebase credentials in issue #214

Co-authored-by: Saurabh Mogre <saurabh.mogre@alleninstitute.org>

…m/mesoscope/cellpack into feature/run-recipes-from-firebase

rugeli

@mogres Thank you for your thorough review! I've just addressed several issues. Please feel free to point out any additional edge cases or concerns that I may overlooked.

rugeli · 2023-12-12T23:02:56Z

cellpack/autopack/loaders/recipe_loader.py

+                        and "all_partners" in obj["partners"]
+                        and not obj["partners"]["all_partners"]
+                    )
+                    else obj.get("partners", [])


Thank you for asking this! The if statement here is to determine whether the recipes retrieved from firebase have partners. During the upload process, the key "all_partners" is added to "partners" of each obj no matter there is partner or not.

The structure of the partners in Firebase is as follows:

for recipes without partners: obj[partners] == {'all_partners': []}

for recipes with partners: obj[partners] == {'all_partners': [<cellpack.autopack.interface_objects.partners.Partner object at xyz>]}

rugeli · 2023-12-12T23:17:13Z

cellpack/autopack/loaders/recipe_loader.py

-        if "gradients" in recipe_data:
+        # gradients in firebase recipes are already stored as a list of dicts
+        if "gradients" in recipe_data and not isinstance(
+            recipe_data["gradients"], list


Just like with Partners, the cellpack system organizes gradients into a list during the upload process. Therefore, we can skip processing gradients in firebase recipes.

rugeli · 2023-12-12T23:36:52Z

cellpack/tests/test_data_doc.py

+def test_is_nested_list():
+    assert DataDoc.is_nested_list([]) is False
+    assert DataDoc.is_nested_list([[], []]) is True
+    assert DataDoc.is_nested_list([[1, 2], [3, 4]]) is True


Good thought! Currently it returns False, though it should return True. While we don't typically encounter such nested lists like [1, [1, 2]] in our recipe data, but you are right that our method should be robust enough to handle all types of nested lists. I just modified the function to run a loop, should be good this time.

rugeli and others added 14 commits June 14, 2023 11:28

[wip] prep recipe data for packing

94ec7cd

get creds from local file

2968f38

save firebase creds to a .creds file

6014501

remove cred arg

fb441f0

check for already handled values in remote recipes

0481d25

can pack one sphere

b5b38c9

* adding username to .creds

e921c35

* formatting

move write_username_to_creds

0a3afff

download recipe testing

2dbfaaf

edit comment

b419a57

Merge branch 'feature/run-recipes-from-firebase' of https://github.co…

f77163d

…m/mesoscope/cellpack into feature/run-recipes-from-firebase

code refactor

a0cd352

lint

bafba75

format tests

a928c1d

rugeli requested review from mogres and meganrm July 13, 2023 19:14

rugeli marked this pull request as draft July 13, 2023 22:58

rugeli force-pushed the feature/run-recipes-from-firebase branch from af55458 to a928c1d Compare July 13, 2023 22:59

rugeli changed the base branch from feature/resovle-grads-in-comp to main July 13, 2023 23:08

rugeli force-pushed the feature/run-recipes-from-firebase branch from 3c0bbf1 to a928c1d Compare August 2, 2023 23:51

rugeli added 8 commits August 18, 2023 14:09

Merge branch 'main' of https://github.com/mesoscope/cellpack into fea…

cc0e19a

…ture/run-recipes-from-firebase

add prep_db_doc

1a8b715

changed class name in DBRecipeHandler

6cad8ee

fix lint and test errors

28af176

initialize firebase handler only once

68660b3

refactor message

14b3876

add remote db options in pack

70e1b12

remove a print statement

09fcbef

rugeli and others added 6 commits September 27, 2023 12:32

Merge branch 'main' of https://github.com/mesoscope/cellpack into fea…

7209086

…ture/run-recipes-from-firebase

Feature/run inherited objects (#198)

cc97646

* turn off resolving inheritance while uploading * able to upload recipes having "inherit" key * get download and pack to work, refactors needed * refactors

Merge branch 'main' of https://github.com/mesoscope/cellpack into fea…

7e2c633

…ture/run-recipes-from-firebase

Merge branch 'feature/run-recipes-from-firebase' of https://github.co…

0d577fe

…m/mesoscope/cellpack into feature/run-recipes-from-firebase

formatting

44c3cb2

testing and refactor

7c0aae3

rugeli changed the title ~~Feature/run recipes from firebase~~ staging: run recipes from firebase Oct 23, 2023

Merge branch 'main' of https://github.com/mesoscope/cellpack into fea…

1f5a0d8

…ture/run-recipes-from-firebase

rugeli mentioned this pull request Oct 23, 2023

Feature/save metadata to firebase #206

Merged

Feature/save metadata to firebase (#206)

6f01cfd

* refactor AWS and firebase handler * databases initiation handling

rugeli marked this pull request as ready for review November 9, 2023 23:19

refactor

3e691a2

rugeli force-pushed the feature/run-recipes-from-firebase branch from 8dcc36c to 3e691a2 Compare November 13, 2023 22:08

mogres reviewed Dec 12, 2023

View reviewed changes

mogres approved these changes Dec 12, 2023

View reviewed changes

rugeli and others added 3 commits December 12, 2023 13:53

Update .gitignore

7364a86

Co-authored-by: Saurabh Mogre <saurabh.mogre@alleninstitute.org>

add file existence check

c4b9786

Merge branch 'feature/run-recipes-from-firebase' of https://github.co…

4e392ea

…m/mesoscope/cellpack into feature/run-recipes-from-firebase

rugeli commented Dec 12, 2023

View reviewed changes

rugeli added 3 commits December 12, 2023 15:44

refactor is_nested_list method

55a313b

revert write_json_file

74c43b3

formatting

93787fc

meganrm approved these changes Dec 15, 2023

View reviewed changes

rugeli merged commit eb4c0fa into main Dec 18, 2023
7 checks passed

rugeli deleted the feature/run-recipes-from-firebase branch December 18, 2023 20:01

rugeli restored the feature/run-recipes-from-firebase branch January 10, 2024 19:19

rugeli deleted the feature/run-recipes-from-firebase branch June 5, 2024 20:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

staging: run recipes from firebase #179

staging: run recipes from firebase #179

rugeli commented Jul 13, 2023 •

edited

Loading

github-actions bot commented Jul 13, 2023 •

edited

Loading

codecov-commenter commented Aug 2, 2023 •

edited

Loading

rugeli commented Nov 29, 2023

mogres left a comment

mogres Sep 14, 2023

rugeli Dec 13, 2023

mogres Dec 12, 2023

rugeli Dec 12, 2023

mogres Dec 12, 2023

rugeli Dec 12, 2023

mogres Dec 12, 2023

rugeli Dec 12, 2023

mogres commented Dec 12, 2023

rugeli commented Dec 12, 2023

rugeli left a comment

rugeli Dec 12, 2023

rugeli Dec 12, 2023

rugeli Dec 12, 2023

staging: run recipes from firebase #179

staging: run recipes from firebase #179

Conversation

rugeli commented Jul 13, 2023 • edited Loading

Problem

Solution

Type of change

Steps to Verify:

Keyfiles (delete if not relevant):

github-actions bot commented Jul 13, 2023 • edited Loading

Packing analysis report

Analysis for packing results located at cellpack/tests/outputs/test_spheres/spheresSST

Packing image

Distance analysis

codecov-commenter commented Aug 2, 2023 • edited Loading

Codecov Report

rugeli commented Nov 29, 2023

mogres left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mogres commented Dec 12, 2023

rugeli commented Dec 12, 2023

rugeli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rugeli commented Jul 13, 2023 •

edited

Loading

github-actions bot commented Jul 13, 2023 •

edited

Loading

codecov-commenter commented Aug 2, 2023 •

edited

Loading