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

Issues with versioning in GitHub builds #800

Closed
b-reyes opened this issue Aug 29, 2022 · 9 comments · Fixed by #804
Closed

Issues with versioning in GitHub builds #800

b-reyes opened this issue Aug 29, 2022 · 9 comments · Fixed by #804
Assignees
Labels
help wanted Extra attention is needed infrastructure
Milestone

Comments

@b-reyes
Copy link
Contributor

b-reyes commented Aug 29, 2022

After merging #799, portions of our tests are failing. After looking into the failing tests, I found that when we install echopype on our GitHub builds, we get this strange versioning: Successfully installed echopype-0.1.dev1+g8b7249c. In the EchoData class we require a certain form for the versioning, see the below snippet.

def version_info(self) -> Tuple[int]:
if self["Provenance"].attrs.get("conversion_software_name", None) == "echopype":
version_str = self["Provenance"].attrs.get("conversion_software_version", None)
if version_str is not None:
version_num = version_str.split(".")[:3]
return tuple([int(i) for i in version_num])

For the tests to pass we need to either correct the above code (so that it accounts for this strange versioning) or we need to somehow correct how the GitHub builds are versioning the install of echopype.

@b-reyes b-reyes added the help wanted Extra attention is needed label Aug 29, 2022
@b-reyes b-reyes added this to the 0.6.3 milestone Aug 29, 2022
@leewujung leewujung moved this to Todo in Echopype Aug 29, 2022
@leewujung
Copy link
Member

@lsetiawan : just bringing this on your radar.

It should be straight forward to use regex instead of split to handle that string parsing. I am curious why the tests didn't have this problem before though -- did something change in the GH actions?

@b-reyes
Copy link
Contributor Author

b-reyes commented Aug 31, 2022

It should be straight forward to use regex instead of split to handle that string parsing. I am curious why the tests didn't have this problem before though -- did something change in the GH actions?

@leewujung after looking through the code, I found that EchoData.version_info is only used in the function ep_version_mapper.py. Additionally, until #799 we had hard coded the version number to 0.6.0, which does not cause an issue in the call EchoData.version_info.

@leewujung
Copy link
Member

until #799 we had hard coded the version number to 0.6.0,

That explains it! But that means we need to fix from the GH actions end.

@b-reyes
Copy link
Contributor Author

b-reyes commented Aug 31, 2022

That explains it! But that means we need to fix from the GH actions end.

Yes, I agree. The best solution would be to make the GH actions install echopype with the version set as the most recent development version.

@emiliom
Copy link
Collaborator

emiliom commented Sep 2, 2022

All the CI tests on PR #799 passed successfully. There are currently no open PR's that are recent, so I checked the most recent closed PR's. #797, closed 3 days ago, also passed all its CI tests.

I do see the incorrect echopype version string all throughout the "Install echopype" section in this CI log in PR #797. But there are no errors reported in those CI test runs.

Did you mean locally run tests? Also, can you specify which tests specifically are failing?

When you install echopype locally, do you get that bad version string too during pip install -e ? This looks to me like a lower-level issue that may go all the way back to setuptools and the related machinery that generate the version string. In fact, I can see that bad string in the CI logs going back as far as July 1 (PR #736). I couldn't go farther back b/c the logs are no longer available.

My assessment is that there's an underlying version string generation problem that's been going on for at least two months but hasn't had any noticeable ramifications anywhere, or at least anything caught by existing tests. PR #799 is simply exposing a problem b/c it introduces a reliance on the version string.

@b-reyes
Copy link
Contributor Author

b-reyes commented Sep 2, 2022

Did you mean locally run tests? Also, can you specify which tests specifically are failing?

@emiliom all PR CI tests should work properly. Locally, everything should be fine too (no bad string with pip install -e). It specifically happens when we merge into dev (I am not sure if it also happens with main or stable). You can see it failing in the GH actions for build.

@emiliom
Copy link
Collaborator

emiliom commented Sep 2, 2022

Got it, thanks.

My observation about the version string being bad as far back as two months ago still applies, though. See this log in a successful build GH action from two months ago:
https://github.com/OSOceanAcoustics/echopype/runs/7133791409?check_suite_focus=true#step:12:22
The echopype version string in the log is "0.1.dev1+gaf68509.d20220630"

It's pretty odd that the same context leads to test failures in the build GH actions but not in the PR GH actions.

@b-reyes
Copy link
Contributor Author

b-reyes commented Sep 2, 2022

It's pretty odd that the same context leads to test failures in the build GH actions but not in the PR GH actions.

I agree. On the bright side, it makes me think that there is hopefully a simple solution.

@lsetiawan lsetiawan linked a pull request Sep 6, 2022 that will close this issue
@lsetiawan
Copy link
Member

See https://github.com/OSOceanAcoustics/echopype/runs/8218144386?check_suite_focus=true#step:11:94. Now CI installs the correct version. Thanks for catching this.

Repository owner moved this from Todo to Done in Echopype Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed infrastructure
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants