NREL / rex

REsource eXtraction Tool (rex)
https://nrel.github.io/rex
BSD 3-Clause "New" or "Revised" License
20 stars 10 forks source link

Fix Bug in `wind-rose` CLI #167

Closed ssolson closed 11 months ago

ssolson commented 11 months ago

setup.py incorrectly specifies the cli path of wind-rose.

Calling wind-rose --help will fail currently.

image

Setting the cli path from wind-rose=rex.jpd.wind_rose_cli:main to wind-rose=rex.joint_pd.wind_rose_cli:main (and reinstalling the package) fixes this: image

ssolson commented 11 months ago

@grantbuster looking at the error and it is not related to my specific change.

I think the actions/checkout-v2 fetch was trying to grab from the origin and not my fork (not 100% sure). image

Because v2 is outdated I updated to v3 and pushed that. In doing so I also removed the fetch depth optimization of only grabbing the head to see if this fixes the issue.

grantbuster commented 11 months ago

Okay, if it fails again maybe rerun with debug logging enabled

ssolson commented 11 months ago

Okay, if it fails again maybe rerun with debug logging enabled

That actions change allowed the that step to pass in Pytests. I will add it to the other workflows (reV, reVX) as well if you are happy with it.

grantbuster commented 11 months ago

@ssolson, only thing i want to verify is that this is indeed checking out the desired repo - e.g. your fork and not the head of rex on main. Can you figure out a way to do a quick negative test to verify this?

ssolson commented 11 months ago

@ssolson, only thing i want to verify is that this is indeed checking out the desired repo - e.g. your fork and not the head of rex on main. Can you figure out a way to do a quick negative test to verify this?

I think that is proven by my commits. The actions on commit c781908 show that only "Pytests" passed because I only updated that action and not "reV Pytests" and "reVX Pytests" actions. This is also how we do it in MHKiT. image

My most recent commits update these actions as well. If you approve that run I expect all tests to pass.

grantbuster commented 11 months ago

@ssolson I'm just wondering how you know what head of rex you're checking out now that you've removed github.event.pull_request.head.ref?

ssolson commented 11 months ago

@grantbuster You guys have setup the action to occur on: pull-request. This assigns the fork/branch making the pull-request as the $GITHUB_WORKSPACE. Checkout will use this value by default. Further you can inspect the logs and see that the expected commit is being merged into the orgin/HEAD:. I have highlighted my commit c78190850b7da83dce642bbc3ba512f9290d6fb0 on line 409 of the checkout logs being merged:

image

https://github.com/NREL/rex/actions/runs/7021817880/job/19104901634

grantbuster commented 11 months ago

Gotcha, cool, thanks for the details!

ssolson commented 11 months ago

Previously I had removed the with: path, but it looks like reV and reVX use these working directories later. My latest commit adds the path designation back for these checkouts. image

grantbuster commented 11 months ago

Previously I had removed the with: path, but it looks like reV and reVX use these working directories later. My latest commit adds the path designation back for these checkouts. image

Hm yeah might be to find the tests dir or test data dir. Thanks!

ssolson commented 11 months ago

Hey Grant I wanted to check in on the rex team's schedule to merge this fix?

grantbuster commented 11 months ago

Hey Grant I wanted to check in on the rex team's schedule to merge this fix?

I approved the PR; I always let the author merge at their convenience in case they're still working on it.

ssolson commented 11 months ago

Ohh okay. I would need write access to the repository to merge the PR. It probably makes the most sense to have you merge the PR for me unless you want to add me as a collaborator to the repo.

image

grantbuster commented 11 months ago

Merged!