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.

Sign up for GitHub

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

Jump to bottom

DM-39747: Use correct Python versioning for PyPI #101

Merged
merged 2 commits into from Jun 21, 2023
Merged

DM-39747: Use correct Python versioning for PyPI #101

merged 2 commits into from Jun 21, 2023

Conversation

arunkannawadi
Copy link
Member

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes

The above checklist is not applicable for this PR.

@codecov
Copy link

codecov bot commented Jun 21, 2023 •
edited

Codecov Report

Patch and project coverage have no change.

Comparison is base (b673666) 85.24% compared to head (31f1911) 85.24%.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #101   +/-   ##
=======================================
  Coverage   85.24%   85.24%           
=======================================
  Files          46       46           
  Lines        3627     3627           
=======================================
  Hits         3092     3092           
  Misses        535      535

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@timj
Copy link
Member

timj commented Jun 21, 2023

That explains why we haven't had a PyPI release since March. Broken in #93. Thanks.

@arunkannawadi arunkannawadi requested a review from timj June 21, 2023 20:57
timj
timj reviewed Jun 21, 2023
View reviewed changes
COPYRIGHT Outdated
Copyright 2015-2016, 2018-2020, 2022-2023 University of Washington
Copyright 2015, 2018-2020 The Board of Trustees of the Leland Stanford Junior University, through SLAC National Accelerator Laboratory
Copyright 2015 The University of Tokyo
Copyright 2008-2014 LSST Corporation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All copyrights were transferred from LSST Corp to AURA so this is not the right fix and the original AURA line above was the right one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So just the last line needs to be removed, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original AURA dates 2008-2014 were fine and this line can be removed.

timj
timj approved these changes Jun 21, 2023
View reviewed changes
Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing the PyPI uploads. Unfortunately a failure in the PyPI action doesn't trigger an email to anyone.

COPYRIGHT Outdated
Copyright 2015-2016, 2018-2020, 2022-2023 University of Washington
Copyright 2015, 2018-2020 The Board of Trustees of the Leland Stanford Junior University, through SLAC National Accelerator Laboratory
Copyright 2015 The University of Tokyo
Copyright 2008-2014 LSST Corporation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original AURA dates 2008-2014 were fine and this line can be removed.

COPYRIGHT Outdated
Copyright 2014-2023 The Trustees of Princeton University
Copyright 2015-2016, 2018-2020, 2022-2023 University of Washington
Copyright 2015, 2018-2020 The Board of Trustees of the Leland Stanford Junior University, through SLAC National Accelerator Laboratory
Copyright 2015 The University of Tokyo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell there is one commit from Tokyo and is 2 lines of code. Generally copyright assignment requires more of a contribution than that and that is why I did not add this line originally (I had already gone through the code to generate the original copyright file contents around 2020). Please remove.

@arunkannawadi arunkannawadi force-pushed the tickets/DM-39747 branch 2 times, most recently from 981f531 to f97b85c Compare June 21, 2023 21:22
@arunkannawadi
Copy link
Member Author

@timj I'll merge after you take one final look at the COPYRIGHT. Since this is used outside of LSST, I wanted to make sure that it's accurate.

timj
timj approved these changes Jun 21, 2023
View reviewed changes
COPYRIGHT Outdated
Copyright 2008-2014, 2016, 2017, 2019, 2020 Association of Universities for Research in Astronomy, Inc. (AURA)
Copyright 2015, 2017, 2018, 2019 The Trustees of Princeton University
Copyright 2015, 2018, 2019 University of Washington
Copyright 2008-2014 Association of Universities for Research in Astronomy, Inc. (AURA)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add back the more modern dates for AURA. I think you went back to just the old LSST Corp dates.

I think it should be:

Suggested change
Copyright 2008-2014 Association of Universities for Research in Astronomy, Inc. (AURA)
Copyright 2008-2014, 2016, 2017, 2019, 2020, 2022-2023 Association of Universities for Research in Astronomy, Inc. (AURA)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I changed it locally and forgot to push

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, the copyright.py script reports an activity from AURA in 2018 as well. Here's one: #28

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided not to include it because it was a trivial change that should not really result in asserting a copyright claim.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if there is a copyright.py script that people are using it needs to be fixed to understand that LSST Corp no longer owns copyright.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I see in the dev guide and it explicitly returns LSST Corporation, despite the docstring mentioning the transfer: https://github.com/lsst-dm/dm_dev_guide/blob/434dec6845dc2ae28eeb9f7c9b918617aff8eee1/legal/copyright.py#L115

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. It's wrong.

timj
timj reviewed Jun 21, 2023
View reviewed changes
README.rst Outdated
@@ -4,6 +4,8 @@ pex_config

.. image:: https://img.shields.io/pypi/v/lsst-pex-config.svg
:target: https://pypi.org/project/lsst-pex-config/
.. image:: https://github.com/lsst/pex_config/actions/workflows/build.yaml/badge.svg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what this gives me? This should always be reporting test passing because we aren't allowed to merge to main without it. Was this showing failed on main because of the PyPI upload failure for any time where main and a tag coincided?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the last step doesn't get run when you merge to main, but only on tag. The status of this badge would have turned to failed (in red). Similar to how the red X shows up next to the most recent commit on main.

This is the workflow log that it would report: https://github.com/lsst/pex_config/actions/runs/5276812239/jobs/9544053729

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still confused because that link at the moment shows "build_and_test: passing" -- it does not show the failure that I can see if I go to the main GitHub page and click on the little red "x" against current main commit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shows green now because the most recent run of this workflow passed (and didn't try uploading to PyPI). If I merged just the badge tonight and hold off the main fix until tomorrow, I believe it would turn red by then. Or it'll turn red automatically if we wait until tomorrow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it always is showing the most recent build from a PR unrelated to main I don't think we want this. It doesn't tell people anything actionable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, removed.

@arunkannawadi arunkannawadi force-pushed the tickets/DM-39747 branch 3 times, most recently from dd74d9b to dd0d983 Compare June 21, 2023 22:21
@arunkannawadi
Use correct Python versioning for PyPI
19a8e10
@arunkannawadi
Update COPYRIGHT
31f1911 
The text is modified from that  generated by the script copyright.py
available from the developer guide.
@arunkannawadi arunkannawadi merged commit 36d8d83 into main Jun 21, 2023
9 checks passed
@arunkannawadi arunkannawadi deleted the tickets/DM-39747 branch June 21, 2023 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timj timj timj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@arunkannawadi @timj