search

TopEFT / topeft

15 stars 24 forks source link

Combine executors #355

Closed ywan2 closed 1 year ago

ywan2 commented 1 year ago

A new script run_executor.py is added in directory analysis/topEFT which combines future executor script run.py and workqueue executor script work_queue_run.py.

codecov[bot] commented 1 year ago

Codecov Report

Merging #355 (cd199d3) into technical_improvements (7c0cb46) will decrease coverage by 0.54%. The diff coverage is 72.00%.

@@                    Coverage Diff                     @@
##           technical_improvements     #355      +/-   ##
==========================================================
- Coverage                   33.89%   33.35%   -0.54%     
==========================================================
  Files                          40       39       -1     
  Lines                        6742     6583     -159     
==========================================================
- Hits                         2285     2196      -89     
+ Misses                       4457     4387      -70
Flag Coverage Δ
unittests 33.35% <72.00%> (-0.54%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
analysis/topEFT/run_topeft.py 73.14% <72.00%> (ø)

... and 1 file with indirect coverage changes

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

kmohrman commented 1 year ago

Thanks very much for adding this @ywan2! It will be really great to have these scripts combined. I will test out the new script soon.

In the meantime, I'm wondering if you'd be able to make the following updates?

  1. I think we could rename the new script run_topcoffea.py (since topcoffea is the processor it runs)
  2. Could you fix the flake8 formatting errors that are causing the CI to fail?
  3. Could you update the Readme in analysis/topEFT to replace the entries for run.py and work_queue_run.py with an entry for the new run_topcoffea.py?
  4. Could you also update the fullR2_run.sh wrapper script to use the new run_topcoffea.py script?
  5. I think we will no longer need the run.py and work_queue_run.py scripts, so these can probably be removed.
bryates commented 1 year ago

Looks like there's 2 CI errors. Flake8 has an issue with a closing parentheses, and we need to update the yml file to point to the new executor to fix this one.

ywan2 commented 1 year ago

I made the changes according to @kmohrman comment. CI is apparently still failling but the close parentheses indent should be fixed. Seems like from @bryates, CI is failing because of other issues on the yaml script but not anything related to run_topcoffea.py correct?

bryates commented 1 year ago

I made the changes according to @kmohrman comment. CI is apparently still failling but the close parentheses indent should be fixed. Seems like from @bryates, CI is failing because of other issues on the yaml script but not anything related to run_topcoffea.py correct?

Yes, sorry, I said yml but it's really in the tests e.g. https://github.com/TopEFT/topcoffea/blob/b87b075daaf00b9c73660b8684f6e1ff8d2b634a/tests/test_futures.py#L10

kmohrman commented 1 year ago

@ywan2 I'm sorry for making the bad suggestion to call the run script run_topcoffea.py. Since the processor is named topeft (not topcoffea) I think it'd make more sense to call it run_topeft.py. So I've pushed that change.

ywan2 commented 1 year ago

@kmohrman It's all good no worries, and thanks for making the changes. Is there anything else significant that caught you for me to edit?

@ywan2 I'm sorry for making the bad suggestion to call the run script run_topcoffea.py. Since the processor is named topeft (not topcoffea) I think it'd make more sense to call it run_topeft.py. So I've pushed that change.

kmohrman commented 1 year ago

@kmohrman It's all good no worries, and thanks for making the changes. Is there anything else significant that caught you for me to edit?

@ywan2 I'm sorry for making the bad suggestion to call the run script run_topcoffea.py. Since the processor is named topeft (not topcoffea) I think it'd make more sense to call it run_topeft.py. So I've pushed that change.

I think it all looks good to me! I'll do a quick check with work queue when I have a chance (probably later this afternoon) and if that looks good I'll be happy to merge at that point, unless @bryates has any further comments.

bryates commented 1 year ago

@kmohrman It's all good no worries, and thanks for making the changes. Is there anything else significant that caught you for me to edit?

@ywan2 I'm sorry for making the bad suggestion to call the run script run_topcoffea.py. Since the processor is named topeft (not topcoffea) I think it'd make more sense to call it run_topeft.py. So I've pushed that change.

I think it all looks good to me! I'll do a quick check with work queue when I have a chance (probably later this afternoon) and if that looks good I'll be happy to merge at that point, unless @bryates has any further comments.

I have nothing else. Go ahead and merge when your checks are done.