add upload_results as a config option #191

rugeli · 2023-09-08T19:27:30Z

Problem

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

Solution

What I/we did to solve this problem

added a key `upload_results in the config file
check config file settings before calling post_and_open_file, to prevent excessive uploads to S3 as well as to avoid opening multiple browser tabs unnecessarily

the action takes place when:

If upload_results is present in config data, it uses whatever value is specified: this allows the user to disable the opening of tabs automatically if they wish.
Or
If upload_results is missing in config data, we check number_of_packings and only upload/post if it is less than 1.

with @mogres

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)

Steps to Verify:

Pack a local recipe with or without the "upload_results" option in the config file

github-actions · 2023-09-08T19:31:24Z

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

mogres · 2023-09-15T23:29:38Z

cellpack/autopack/writers/__init__.py

@@ -175,7 +175,8 @@ def save_as_simularium(self, env, all_ingr_as_array, compartments):
        file_name = env.helper.writeToFile(
            env.result_file, env.boundingBox, env.name, env.version
        )
-        autopack.helper.post_and_open_file(file_name)
+        if env.config_data is None or env.config_data.get("upload_results", True):


This check is a bit confusing for me. We're uploading and posting results if the config_data is missing or if upload_results is not set?

I would consider adding another check on number_of_packings to make sure we don't end up opening multiple browser tabs if upload_results is not set or missing on a config with 100s of packings.

Suggested change

if env.config_data is None or env.config_data.get("upload_results", True):

if env.config_data is None or env.config_data.get("upload_results", env.config_data.get("number_of_packings", 1) <= 1):

Thank you for your suggestion of adding a check for number_of_packings! If number_of_packings is always going to be specified in parallel packings, then we should be able to prevent opening multiple browser tabs by just checking the number_of_packings key.

That way, we wouldn't need to worry about adding the upload_results key to the config.

For example, we could use if env.config_data.get("number_of_packings", 1) <= 1):, just as you suggested

mogres · 2023-09-18T21:42:48Z

cellpack/autopack/writers/__init__.py

@@ -175,7 +175,8 @@ def save_as_simularium(self, env, all_ingr_as_array, compartments):
        file_name = env.helper.writeToFile(
            env.result_file, env.boundingBox, env.name, env.version
        )
-        autopack.helper.post_and_open_file(file_name)
+        if env.config_data.get("number_of_packings", 1) <= 1:


Thanks for incorporating this! However, I think keeping the upload_results option is still a good idea and we should only fall back on number_of_packings check if upload_results is missing from the config.

My suggestion was:
if env.config_data is None or env.config_data.get("upload_results", env.config_data.get("number_of_packings", 1) <= 1):
which only uploads results and opens a tab if following conditions are met:

Config data is missing
Or

If upload_results is present in config data, it uses whatever value is specified: this allows the user to disable the opening of tabs automatically if they wish. Having this option would be useful in my opinion.
Or

If upload_results is missing in config data, we check number_of_packings and only upload/post if it is less than 1.
Let me know if this makes sense!

meganrm · 2023-09-22T20:01:09Z

cellpack/autopack/writers/__init__.py

-        autopack.helper.post_and_open_file(file_name)
+        if env.config_data is None or env.config_data.get(
+            "upload_results", env.config_data.get("number_of_packings", 1) <= 1
+        ):


two things: I don't thin env.config_data can ever be none, so I don't think you need that check
I think you could do 3 or less. I have run some multiple packings recently that I would like to see the results of.

meganrm · 2023-09-22T20:01:43Z

examples/packing-configs/peroxisome_packing_config.json

@@ -14,5 +14,6 @@
    "spacing": 2.5,
    "use_periodicity": false,
    "show_sphere_trees": false,
-    "load_from_grid_file": true
+    "load_from_grid_file": true,
+    "upload_results": false


all config settings should also have a default setting in the config loader

codecov-commenter · 2023-09-25T19:57:58Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (58c0256) 98.52% compared to head (17fd02f) 98.52%.

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

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #191   +/-   ##
=======================================
  Coverage   98.52%   98.52%           
=======================================
  Files          16       16           
  Lines         476      476           
=======================================
  Hits          469      469           
  Misses          7        7

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

….com/mesoscope/cellpack into feature/upload-results-config-option

mogres

Looks good to me! I think we can build upon this by using a single simularium file with multiple packings using time as an index.

Should be good to merge once conflicts are resolved!

rugeli added 8 commits August 29, 2023 15:02

upload local recipes to S3

f14bb6e

update README and add boto3 into requirements

59b80b4

lint

565487c

rerange import order

c97d5a8

add save_file for S3 uploads and url generation

d09b64a

delete redundant error msg

2b22d68

add "upload_results" in config files as an option

390d284

conditionally call post_and_open_file

e96a996

rugeli requested review from mogres and meganrm September 8, 2023 19:27

rugeli mentioned this pull request Sep 14, 2023

staging: run recipes from firebase #179

Merged

mogres reviewed Sep 15, 2023

View reviewed changes

mogres reviewed Sep 18, 2023

View reviewed changes

rugeli force-pushed the feature/upload-results-config-option branch from 919d9a4 to e96a996 Compare September 18, 2023 23:04

add check number_of_packings

6cda9e7

meganrm reviewed Sep 22, 2023

View reviewed changes

Base automatically changed from feature/upload-local-recipes-to-S3 to main September 22, 2023 21:18

rugeli and others added 2 commits September 25, 2023 12:45

update conditional statement

c5b9ec7

Merge branch 'main' into feature/upload-results-config-option

89e9297

rugeli added 2 commits September 25, 2023 12:58

update

fa3665f

Merge branch 'feature/upload-results-config-option' of https://github…

0ee1fb0

….com/mesoscope/cellpack into feature/upload-results-config-option

rugeli requested review from meganrm and mogres September 25, 2023 20:10

meganrm approved these changes Sep 26, 2023

View reviewed changes

mogres approved these changes Sep 26, 2023

View reviewed changes

Merge branch 'main' into feature/upload-results-config-option

17fd02f

rugeli merged commit fee423f into main Sep 27, 2023
7 checks passed

rugeli deleted the feature/upload-results-config-option branch September 27, 2023 00:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add upload_results as a config option #191

add upload_results as a config option #191

rugeli commented Sep 8, 2023 •

edited

Loading

github-actions bot commented Sep 8, 2023 •

edited

Loading

mogres Sep 15, 2023

rugeli Sep 18, 2023 •

edited

Loading

mogres Sep 18, 2023 •

edited

Loading

meganrm Sep 22, 2023

meganrm Sep 22, 2023

codecov-commenter commented Sep 25, 2023 •

edited

Loading

mogres left a comment •

edited

Loading

	if env.config_data is None or env.config_data.get("upload_results", True):
	if env.config_data is None or env.config_data.get("upload_results", env.config_data.get("number_of_packings", 1) <= 1):

add upload_results as a config option #191

add upload_results as a config option #191

Conversation

rugeli commented Sep 8, 2023 • edited Loading

Problem

Solution

Type of change

Steps to Verify:

github-actions bot commented Sep 8, 2023 • edited Loading

Packing analysis report

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

Packing image

Distance analysis

mogres Sep 15, 2023

Choose a reason for hiding this comment

rugeli Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

mogres Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

meganrm Sep 22, 2023

Choose a reason for hiding this comment

meganrm Sep 22, 2023

Choose a reason for hiding this comment

codecov-commenter commented Sep 25, 2023 • edited Loading

Codecov Report

mogres left a comment • edited Loading

Choose a reason for hiding this comment

rugeli commented Sep 8, 2023 •

edited

Loading

github-actions bot commented Sep 8, 2023 •

edited

Loading

rugeli Sep 18, 2023 •

edited

Loading

mogres Sep 18, 2023 •

edited

Loading

codecov-commenter commented Sep 25, 2023 •

edited

Loading

mogres left a comment •

edited

Loading