ENH: Add optional `anaconda_nightly_upload_organization` argument #47

skoudoro · 2023-11-29T18:38:26Z

This is a follow up and fixes #46

This PR goal is to allow other Scientific Python projects to use this github action.

Please, let me know what you think, and I will be happy to rename the variables if needed.

Thanks

Squash and merge commit

* Add optional `anaconda_nightly_upload_organization` and
  `anaconda_nightly_upload_labels` arguments to allow for packages
  to be uploaded to custom registires with the GitHub Action.
* Add docs to README providing an example of how to upload to an
  alternative registry.

NF: add anaconda_nightly_upload_token paremeter DOC: Add associated documentation

tupui

Nice 👍 thank you for the PR. LGTM but I am not coding actions often so another person with more experience there should also have a close look.

matthewfeickert

I'm on a plane and so reviewing on the GitHub mobile app (which isn't the best, sorry) but can you elaborate on why labels are needed now? They shouldn't be AFAIK.

matthewfeickert · 2023-11-29T18:47:40Z

action.yml

+  anaconda_nightly_upload_labels:
+    description: 'Labels assigned to the uploaded artifacts'
+    required: false
+    default: main


Why is the label needed? This is a change in current behavior.

By default, the label is main when you upload an artifact in anaconda

in our case (https://anaconda.org/dipy/dipy), the organization name is not explicit. I would like to avoid random people downloading nightly wheels and believing that is it a release. So, we tag them as dev instead of main (see the difference between the sreenshot above and below

one artifact can have one or multiple labels. I am fine for alternative solution to avoid confusion with users (since I can relabel all the old artifacts)

Documentation on anaconda label: https://docs.anaconda.com/free/anacondaorg/user-guide/work-with-labels/

matthewfeickert · 2023-11-29T19:04:07Z

I'm on a plane and so reviewing on the GitHub mobile app (which isn't the best, sorry) but can you elaborate on why labels are needed now? They shouldn't be AFAIK.

Oh I guess this isn't a change in behavior given the current channel uploads

My bad. Can you document this optional variable though in your example?

stefanv

Thanks @skoudoro! It looks good to me; I left some feedback.

README.md

stefanv · 2023-11-29T20:26:50Z

README.md

+        artifacts_path: dist
+        anaconda_nightly_upload_url: my-alternative-scientific-channel
+        anaconda_nightly_upload_token: ${{secrets.UPLOAD_TOKEN}}
+        anaconda_nightly_upload_labels: main


Suggested change

anaconda_nightly_upload_labels: main

anaconda_nightly_upload_labels: main

This is the default, so how about using a different label in the example?

agree, I will update that

action.yml

stefanv · 2023-11-29T20:28:07Z

action.yml

+  anaconda_nightly_upload_url:
+    description: 'URL to upload to scientific python org'
+    required: false
+    default: scientific-python-nightly-wheels


Suggested change

default: scientific-python-nightly-wheels

default: scientific-python-nightly-wheels

I don't know how the action works, but this is confusing since it isn't a URL.
If it can be anything other than a URL, that should either be documented or the field be renamed.

So the variable name is wrong! it should be anaconda_nightly_upload_organization

removing the default will break downstream packages using this action as soon as it is merged like scikit-image

scikit-image is using main. Maybe we can update this in a new PR and let some time to all downstream package to add/update this variable.

action.yml

Co-authored-by: Stefan van der Walt <[email protected]>

- rename anaconda_nightly_upload_url to anaconda_nightly_upload_organization

stefanv

Looks good to me; I'll let @matthewfeickert do the final review and merge.

matthewfeickert · 2023-11-29T22:10:44Z

Looks good to me; I'll let @matthewfeickert do the final review and merge.

Thanks all! 👍 I'm running in airports so if I have wifi on next flight I'll knock this final look over out and if not then I'll get to it tomorrow afternoon Europe time.

matthewfeickert · 2023-12-01T00:39:10Z

action.yml

+  anaconda_nightly_upload_labels:
+    description: 'Labels assigned to the uploaded artifacts'
+    required: false
+    default: [main]


@skoudoro Hey sorry to have one final question here (now that I've arrived), but can you point me to a reference on how you know that anaconda upload --label will accept a list of labels (as opposed to repeated calls of --label like how you would have to do with --channel with micromamba install? micromamba install --channel main --channel conda-forge numpy)?

The anaconda-client docs are not good, so it is possible I'm missing something. Have you tested this with any uploads to your registry yet?

can you point me to a reference on how you know that anaconda upload --label will accept a list of labels

As you can see below, I just used the help of the command line (anaconda upload --help.

There is also the following documentation : https://docs.anaconda.com/anaconda-repository/user-guide/tutorials/#using-labels-in-the-development-cycle

Have you tested this with any uploads to your registry yet?

Yes, I tried with some artifacts on our repository. I use the following command manually:
anaconda upload --force --user dipy --label dev --label main *1.6.0*.whl

Not sure how to handle the list with the docker. I am looking forward for your feedback

something is still wrong here, see the "Sequence was not expected" error in the CI: https://github.com/scientific-python/upload-nightly-action/actions/runs/7038337722/job/19199259041#step:9:1

Yeah @bsipocz's point is exactly my concern here. [main] is a sequence but @skoudoro the example you gave of

anaconda upload --force --user dipy --label dev --label main *1.6.0*.whl

demonstrates the opposite of what you said. This is not a list, this is repeated calls of --label.

@skoudoro can you tell us more about how you're using labels now and why you want to have multiple labels?

yes, I just updated the code. This is not a list, sorry for the confusion.

how you're using labels now and why you want to have multiple labels?

We use only the label dev for now

in our case (https://anaconda.org/dipy/dipy), the organization name is not explicit for nightly wheels. I would like to avoid random people downloading nightly wheels and believing that is it a release. So, we tag them as dev instead of main. it force user to be explicit and conscious of what they are trying to get. At the same time, it is not an issue for downstream packages.

we do not use multiple labels as the same time. the line above was just an example to show you how it can be done.

bsipocz · 2023-12-01T01:55:14Z

cmd.sh

-ANACONDA_ORG="scientific-python-nightly-wheels"
+ANACONDA_ORG="${INPUT_ANACONDA_NIGHTLY_UPLOAD_ORGANIZATION}"


would this break the current behaviour for the core libraries uploading to the default location?

No, because I added the default location in this line: https://github.com/scientific-python/upload-nightly-action/pull/47/files#diff-1243c5424efaaa19bd8e813c5e6f6da46316e63761421b3e5f5c8ced9a36e6b6R20

However, I believe that:

this parameter should be required instead of optional

After merging, we should ask the core libraries to add and update this variable

Then, when it is done, create a new PR here to remove this default location and let it empty.

Thank you for your review @bsipocz!

@skoudoro While I'm not against the idea of being explicit about the target channel (I think that's a good idea in general to guard against default changes), I think the original intent for the GitHub Action was to make uploading as easy as possible for the core packages with the idea that they need to configure things as minimally as possible.

I think that this could probably be discussed more broadly outside of this PR though, so maybe you can open up an Issue from your comment to discuss what the target audience and scope of the action configuration should be?

matthewfeickert · 2023-12-01T10:18:56Z

action.yml

+  anaconda_nightly_upload_labels:
+    description: 'Labels assigned to the uploaded artifacts'
+    required: false
+    default: [main]


Suggested change

default: [main]

default: main

This should be a string and not a sequence (c.f. #47 (comment))

Yes, agree.

I just read more about that and make sense

matthewfeickert · 2023-12-08T16:26:13Z

(Sorry, been traveling for work to CERN and so haven't gotten back to reviewing this. Will try this weekend.)

skoudoro · 2023-12-08T16:27:51Z

No worries @matthewfeickert , take your times

- parse string in case of multiple labels

skoudoro · 2023-12-09T13:44:01Z

To help you for the review I use this action here: https://github.com/dipy/dipy/blob/master/.github/workflows/nightly.yml#L136-L156

and an example of the action result: https://github.com/dipy/dipy/actions/runs/7148367041/job/19471063289#step:4:376

bsipocz · 2024-01-04T19:58:09Z

ping @matthewfeickert, this would be good to have in sooner rather than later to help various downstream wheel production

matthewfeickert · 2024-01-04T20:17:53Z

ping @matthewfeickert, this would be good to have in sooner rather than later to help various downstream wheel production

Yes, I'm aware but c.f. again #50 (comment) for why I've been preoccupied. I'll have a stable computer and get to this before end of week.

bsipocz · 2024-01-04T20:59:38Z

c.f. again #50 (comment) for why I've been preoccupied

Hah, fair enough, sorry for rushing you. I was also traveling, so I didn't really internalize all the comments from the past few weeks.

matthewfeickert

@skoudoro Okay, sorry for a ridiculously long delay (😞) in this review from me (I've been without a working computer for multiple weeks). So I think this is overall good and the right approach, but the one important change is that this approach currenlty assume a space seperated list of labels, which is not a common assumption in the world of GitHub Actions where newline seperted or comma seperated lists are generally expected.

The suggested changes I've added in this review address that and change the logic to be able to handle newline or comma seperated lists of lables. I've tried to step through things to make the reasoning/motivation clear, but if you have any questions please feel free to ask (I promise I will be much more responsive now!).

action.yml

cmd.sh

stefanv · 2024-01-23T19:44:06Z

In general, I would urge reviewers who want to see major refactors when a PR is close to its conclusion to push changes (when confident) or make PRs (of they want the contributor to review). This helps to keep things moving, and preserves contributor motivation.

skoudoro · 2024-01-23T20:07:30Z

Sorry, I was traveling and then sick. I will get back to it by the end of the week (trying to catch up with everything). Apologize for that

bsipocz · 2024-01-23T20:14:34Z

In general, I would urge reviewers who want to see major refactors when a PR is close to its conclusion to push changes (when confident) or make PRs (of they want the contributor to review). This helps to keep things moving, and preserves contributor motivation.

I'm happy to do this (and am agree and tend to do this in places where I'm the primary maintainer and only have non-controversial or bikeshed comments), but feel that overtaking a PR where the contributor said they would come back to address another reviewer's comments can perceived as aggressive rather than helpful and I definitely would like to avoid that.

This PR was stuck on the reviewers' side for long enough that there is nothing to apologize for @skoudoro!

matthewfeickert · 2024-01-23T20:37:45Z

In general, I would urge reviewers who want to see major refactors when a PR is close to its conclusion to push changes (when confident) or make PRs (of they want the contributor to review). This helps to keep things moving, and preserves contributor motivation.

@stefanv I think I share @bsipocz's feelings here, in that I am not a big fan of reviewrs pushing changes to code unless explictly asked to, which is why I always use GitHub's suggest changes tool so that if people agree with them they can incoperate as many as they wish into the PR with a simple button click and don't have to do any additional work themselves.

If pushing changes is preferred here I'm fine with that, and I will just accept my own proposed changes through the UI as a single commit. I'm happy to wait though to let @skoudoro have time to look at the proposed changes later this week.

stefanv · 2024-01-23T20:48:28Z

Using the inline commenting system is indeed friendly in that regard, but when larger pieces of code needs to be re-written it becomes impractical and a PR to the contributor branch does the same.

We should, however, not be shy to help one another shoulder the load; I've never heard of a maintainer complain because other maintainers helped them too much. We all have long TODO lists that will never be completed in this lifetime!

bsipocz · 2024-01-23T21:15:01Z

We should, however, not be shy to help one another shoulder the load; I've never heard of a maintainer complain because other maintainers helped them too much.

fair enough, my experience with people being annoyed about PRs being taken over was PRs from generic (scientist or student) contributors rather than PRs from maintainers of other packages

matthewfeickert · 2024-01-23T21:28:46Z

We should, however, not be shy to help one another shoulder the load; I've never heard of a maintainer complain because other maintainers helped them too much. We all have long TODO lists that will never be completed in this lifetime!

Okay, given this I'm going to accept my suggested changes as a single commit. @skoudoro to be explicit here, please take this as just a suggestion and feel free to revert or make any futher changes that you think would improve things. I'm happy to iterate with you further if needed! (though in full disclousure I have a job interview next week that is going to take me offline for most of it 😬 ).

Using space seperated lists is not standard across GitHub Actions, which in general assume that you're providing a newline seperatd or comma seperated list (c.f. https://github.com/docker/build-push-action?tab=readme-ov-file#inputs as an example). So if we agree that we should be expecting newline seperated or comma seperated inputs then we can just treat all input the same by: First, converting all input into a comma separated string Then, create an array of the comma seperated labels Finally, parse that array into a single string that represents all the label arguments to be included (you were already doing this part)

stefanv · 2024-01-23T21:35:06Z

Thanks @matthewfeickert, and good luck with the interview!

stefanv · 2024-01-23T21:37:27Z

Test failure 👀

ANACONDA_TOKEN is empty , exiting...

matthewfeickert · 2024-01-23T21:46:28Z

Test failure 👀

ANACONDA_TOKEN is empty , exiting...

That's expected. c.f. #32 (comment) for a not great summary, but basically there isn't a great solution at the moment to run the tests on PRs from froks that wouldn't potentially open up a security threat.

I'm not saying it is impossible to fix, but it would require a redesign of the CI.

(edit: I would be thrilled to learn that I'm wrong here, so please do take a look if you're inclined!)

stefanv · 2024-01-23T22:36:20Z

That makes sense; I run into this all the time.

bsipocz · 2024-01-24T00:13:53Z

OK, so let's have this in now, thank you so much for all the efforts. If needed further adjustments can be done in follow-up PRs.

* Use the correct shell variable "LABEL_ARGS" to pass the lable args to the `anaconda upload` command. - Amends PR #47 * Note that it is important that ${LABEL_ARGS} is NOT quoted during shell parameter expansion, else it will be treated as a file path to anaconda upload and not an argument. - e.g. This will trigger `File "--label main " does not exist` errors.

matthewfeickert · 2024-01-24T01:42:50Z

@skoudoro Can you verify that if you use

jobs:
  steps:
    ...
    - name: Upload wheel
      uses: scientific-python/upload-nightly-action@3eb3a42b50671237cace9be2d18a3e4b3845d3c4  # currently main
      with:
        artifacts_path: dist
        anaconda_nightly_upload_organization: my-alternative-organization
        anaconda_nightly_upload_token: ${{secrets.UPLOAD_TOKEN}}
        anaconda_nightly_upload_labels: dev

that things work for you to upload to your alternative registry? If so, I'll tag 3eb3a42 as v0.3.0.

bsipocz · 2024-01-24T01:48:15Z

cc @nabobalis if you want to try this out for openastronomy/sunpy

skoudoro · 2024-01-24T15:36:09Z

Thank you all for pushing that forward.

@matthewfeickert, I just pushed the changes on dipy/dipy@66d08be. This is running

nabobalis · 2024-01-24T17:10:45Z

cc @nabobalis if you want to try this out for openastronomy/sunpy

Let me try.

skoudoro · 2024-01-24T17:14:09Z

@skoudoro Can you verify that if you use

jobs:
  steps:
    ...
    - name: Upload wheel
      uses: scientific-python/upload-nightly-action@3eb3a42b50671237cace9be2d18a3e4b3845d3c4  # currently main
      with:
        artifacts_path: dist
        anaconda_nightly_upload_organization: my-alternative-organization
        anaconda_nightly_upload_token: ${{secrets.UPLOAD_TOKEN}}
        anaconda_nightly_upload_labels: dev

that things work for you to upload to your alternative registry? If so, I'll tag 3eb3a42 as v0.3.0.

ok, it works ! Thanks @matthewfeickert ! Make sure to put the right commit number @nabobalis. The little code above was confusing (wrong commit number)

(edited by @matthewfeickert to avoid having invalid commit numbers provided by him floating about waiting to cause copy-paste errors)

matthewfeickert · 2024-01-24T17:20:35Z

The little code above was confusing (wrong commit number)

@skoudoro what's wrong with 3eb3a42 ?

(edit: Ah that I had typoed a f at the end! Fixed above)

matthewfeickert · 2024-01-24T18:42:37Z

Okay, great as things seem to be working for dipy's uploads

I'll go ahead and make a new tag and release.

Thank you for the PR and for verifying this works, @skoudoro!

NF: Add anaconda_nightly_upload_url parameter

e50044e

NF: add anaconda_nightly_upload_token paremeter DOC: Add associated documentation

skoudoro mentioned this pull request Nov 29, 2023

Allow a new inputs parameter named anaconda_nightly_upload_url #46

Closed

tupui approved these changes Nov 29, 2023

View reviewed changes

matthewfeickert reviewed Nov 29, 2023

View reviewed changes

DOC: add labels documentation

66d2324

stefanv reviewed Nov 29, 2023

View reviewed changes

skoudoro and others added 2 commits November 29, 2023 15:34

Apply suggestions from code review

317d7cb

Co-authored-by: Stefan van der Walt <[email protected]>

- address stefanv comments.

9740248

- rename anaconda_nightly_upload_url to anaconda_nightly_upload_organization

stefanv approved these changes Nov 29, 2023

View reviewed changes

matthewfeickert requested changes Dec 1, 2023

View reviewed changes

matthewfeickert changed the title ~~NF: Add anaconda_nightly_upload_url~~ ENH: Add optional anaconda_nightly_upload_organization argument Dec 1, 2023

matthewfeickert assigned skoudoro Dec 1, 2023

matthewfeickert added documentation Improvements or additions to documentation enhancement New feature or request labels Dec 1, 2023

bsipocz reviewed Dec 1, 2023

View reviewed changes

matthewfeickert requested changes Dec 1, 2023

View reviewed changes

skoudoro force-pushed the new-paramater branch 2 times, most recently from bdca3eb to d6241ac Compare December 8, 2023 15:50

FIX: del list, docker handle only str/bool/number

9b762da

- parse string in case of multiple labels

skoudoro force-pushed the new-paramater branch from d6241ac to 9b762da Compare December 8, 2023 21:28

bsipocz mentioned this pull request Jan 1, 2024

Request access for sunpy #50

Closed

matthewfeickert requested changes Jan 6, 2024

View reviewed changes

action.yml Outdated Show resolved Hide resolved

cmd.sh Outdated Show resolved Hide resolved

cmd.sh Outdated Show resolved Hide resolved

matthewfeickert approved these changes Jan 23, 2024

View reviewed changes

bsipocz approved these changes Jan 24, 2024

View reviewed changes

bsipocz merged commit f689fba into scientific-python:main Jan 24, 2024
1 of 2 checks passed

This was referenced Jan 24, 2024

CI tests failing with Error: File "main" does not exist for labeled file upload #52

Closed

FIX: Use correct shell variable in upload #53

Merged

matthewfeickert mentioned this pull request Jan 24, 2024

CI: Add test for uploads with multiple labels #54

Merged

skoudoro deleted the new-paramater branch January 24, 2024 15:36

matthewfeickert mentioned this pull request Feb 8, 2024

Make Remove Old Wheels workflow seperate GitHub Action #62

Open

	anaconda_nightly_upload_labels: main
	anaconda_nightly_upload_labels: main

	default: scientific-python-nightly-wheels
	default: scientific-python-nightly-wheels

		ANACONDA_ORG="scientific-python-nightly-wheels"
		ANACONDA_ORG="${INPUT_ANACONDA_NIGHTLY_UPLOAD_ORGANIZATION}"

ENH: Add optional anaconda_nightly_upload_organization argument #47

ENH: Add optional anaconda_nightly_upload_organization argument #47

Conversation

skoudoro commented Nov 29, 2023 • edited by matthewfeickert Loading

tupui left a comment

Choose a reason for hiding this comment

matthewfeickert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewfeickert commented Nov 29, 2023

stefanv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanv left a comment

Choose a reason for hiding this comment

matthewfeickert commented Nov 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewfeickert commented Dec 8, 2023

skoudoro commented Dec 8, 2023

skoudoro commented Dec 9, 2023

bsipocz commented Jan 4, 2024

matthewfeickert commented Jan 4, 2024

bsipocz commented Jan 4, 2024

matthewfeickert left a comment • edited Loading

Choose a reason for hiding this comment

stefanv commented Jan 23, 2024

skoudoro commented Jan 23, 2024

bsipocz commented Jan 23, 2024 • edited Loading

matthewfeickert commented Jan 23, 2024

stefanv commented Jan 23, 2024

bsipocz commented Jan 23, 2024

matthewfeickert commented Jan 23, 2024

stefanv commented Jan 23, 2024

stefanv commented Jan 23, 2024

matthewfeickert commented Jan 23, 2024 • edited Loading

stefanv commented Jan 23, 2024

bsipocz commented Jan 24, 2024

matthewfeickert commented Jan 24, 2024 • edited Loading

bsipocz commented Jan 24, 2024

skoudoro commented Jan 24, 2024

nabobalis commented Jan 24, 2024

skoudoro commented Jan 24, 2024 • edited by matthewfeickert Loading

matthewfeickert commented Jan 24, 2024 • edited Loading

matthewfeickert commented Jan 24, 2024

ENH: Add optional `anaconda_nightly_upload_organization` argument #47

ENH: Add optional `anaconda_nightly_upload_organization` argument #47

skoudoro commented Nov 29, 2023 •

edited by matthewfeickert

Loading

matthewfeickert left a comment •

edited

Loading

bsipocz commented Jan 23, 2024 •

edited

Loading

matthewfeickert commented Jan 23, 2024 •

edited

Loading

matthewfeickert commented Jan 24, 2024 •

edited

Loading

skoudoro commented Jan 24, 2024 •

edited by matthewfeickert

Loading

matthewfeickert commented Jan 24, 2024 •

edited

Loading