Skip to content

Conversation

@piotr-roslaniec
Copy link
Contributor

@piotr-roslaniec piotr-roslaniec commented May 30, 2023

Type of PR:

  • Feature

Required reviews:

  • 1

What this does:

  • Bumps rust-umbral version

Why it's needed:

@codecov-commenter
Copy link

Codecov Report

Merging #57 (fda5ed6) into main (6a16679) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #57   +/-   ##
=======================================
  Coverage   15.25%   15.25%           
=======================================
  Files          16       16           
  Lines        2845     2845           
=======================================
  Hits          434      434           
  Misses       2411     2411           

@piotr-roslaniec piotr-roslaniec marked this pull request as ready for review May 30, 2023 10:45
Copy link
Member

@derekpierre derekpierre left a comment

Choose a reason for hiding this comment

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

Is this a reason to bump the version of nucypher-core? i.e. do we need to do a new nucypher-core release, or we are still in the process of testing changes?

As a side note, I intend to bump the release version for PR #54 .

@piotr-roslaniec
Copy link
Contributor Author

I think we're still in the process of testing. Once we have nucypher-ts and nucypher working end-to-end, I think it would be a good time to release nucypher-core. We can always release an alpha version if we want to make nucypher and nucypher-ts integration a bit easier.

Copy link
Member

@derekpierre derekpierre left a comment

Choose a reason for hiding this comment

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

🎸

@derekpierre
Copy link
Member

derekpierre commented May 30, 2023

I think we're still in the process of testing. Once we have nucypher-ts and nucypher working end-to-end, I think it would be a good time to release nucypher-core. We can always release an alpha version if we want to make nucypher and nucypher-ts integration a bit easier.

Wait. Thinking about this more...

Do you intend to merge this change before the testing is completed, OR are you testing currently and will update this PR as needed and merge when testing/other repos related changes are completed?

@piotr-roslaniec
Copy link
Contributor Author

From my perspective, I don't see any additional changes in rust-umbral I could need. I only need to resolve certain dependency conflicts. You can see in Cargo.toml that this change was already tested prior to 0.10.0 release.

@derekpierre
Copy link
Member

From my perspective, I don't see any additional changes in rust-umbral I could need. I only need to resolve certain dependency conflicts. You can see in Cargo.toml that this change was already tested prior to 0.10.0 release.

The concern I have is, do these changes have downstream effects on nucypher/nucypher and are those being done in tandem? If there are downstream effects, we should have those as a PR in nucypher/nucypher before actually merging this change.

@piotr-roslaniec
Copy link
Contributor Author

Let's postpone merging until I sort out the downstream changes (if needed).

@piotr-roslaniec
Copy link
Contributor Author

I think we can merge this after successful merge of nucypher/nucypher#3135 and merge of downstream changes in other PRs

@derekpierre
Copy link
Member

I think we can merge this after successful merge of nucypher/nucypher#3135 and merge of downstream changes in other PRs

I'm confused about the ordering. Doesn't this PR need to be merged/released first and then 3135 updated to depend on it?

@piotr-roslaniec
Copy link
Contributor Author

Sorry, let me clarify:

  1. #3155 is a canary for testing changes in this PR (and other PRs)
  2. If #3155 is merged, then it would mean this PR checks out
  3. At some point we'll release a new version of nucypher-core and introduce it to nucypher, and replace dependency on the pre-release package (in nucypher/requirements.txt) with a proper nucypher-core package.

@derekpierre
Copy link
Member

derekpierre commented Jun 2, 2023

Sorry, let me clarify:

  1. #3155 is a canary for testing changes in this PR (and other PRs)
  2. If #3155 is merged, then it would mean this PR checks out
  3. At some point we'll release a new version of nucypher-core and introduce it to nucypher, and replace dependency on the pre-release package (in nucypher/requirements.txt) with a proper nucypher-core package.

Yep, but I don't think 3135 should be merged first. It should be used for testing no doubt, but merging when using a forked/branch github dependency on nucypher-core can cause issues.

  1. #3155 is a canary for testing changes in this PR (and other PRs)

You know that the changes will likely work after merging nucypher-core first because you are depending on a specific commit, and that commit holds after merging.

I think the options are:

  1. merge the nucypher-core changes, do a release, then update 3135 to depend on that release. This is what I thought we've been doing until now - but may it is cumbersome for quick iterative development. If we want to change that to reduce nucypher-core releases that's fine.

OR

  1. If we want to collect various changes in nucypher-core before a release is actually done, merge the nucypher-core change first and have the nucypher PR update the dependency to the nucypher-core:main commit in the github repos (and not a forked/branched version). If we are going to do this then the Pipfile should be changed as well to make that clear and not just the requirements.txt/dev-requirements.txt

cc @KPrasch in case he has any other ideas.

@piotr-roslaniec
Copy link
Contributor Author

I'd prefer option 2). And yes, keeping a fork in nucypher dependencies is not really an option.

@fjarri fjarri merged commit 92e7647 into nucypher:main Jun 2, 2023
@piotr-roslaniec piotr-roslaniec deleted the bump-umbral-pre branch June 3, 2023 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants