nucypher / nucypher-core

Core structures for Nucypher network in Rust
6 stars 11 forks source link

Bump `umbral-pre` to `0.10.0` #57

Closed piotr-roslaniec closed 1 year ago

piotr-roslaniec commented 1 year ago

Type of PR:

Required reviews:

What this does:

Why it's needed:

codecov-commenter commented 1 year ago

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 commented 1 year ago

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.

derekpierre commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

piotr-roslaniec commented 1 year ago

I think we can merge this after successful merge of https://github.com/nucypher/nucypher/pull/3135 and merge of downstream changes in other PRs

derekpierre commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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