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

chore: split NadaType and NadaValue / refactoring #60

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Jmgr
Copy link
Contributor

@Jmgr Jmgr commented Nov 20, 2024

Fixes #
Design discussion issue (if applicable) #

Changes

Please provide a brief description of the changes here.

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Backwards compatibility analysis completed (if applicable). "Will this change require recompilation and upload of user programs?"

@pablojhl
Copy link
Member

I was taking a look at the PR and I'm confused. What is a NadaValue at the compiling level exactly? IMO NadaValue should exist only at the runtime level, right?

@Jmgr
Copy link
Contributor Author

Jmgr commented Nov 21, 2024

I was taking a look at the PR and I'm confused. What is a NadaValue at the compiling level exactly? IMO NadaValue should exist only at the runtime level, right?

In this context a NadaValue is what you would pass to Tuple.new for instance: PublicInteger or Array. A NadaType would be PublicIntegerType or ArrayType. The two are currently mixed in the code, which creates some confusion.

@pablojhl
Copy link
Member

I was taking a look at the PR and I'm confused. What is a NadaValue at the compiling level exactly? IMO NadaValue should exist only at the runtime level, right?

In this context a NadaValue is what you would pass to Tuple.new for instance: PublicInteger or Array. A NadaType would be PublicIntegerType or ArrayType. The two are currently mixed in the code, which creates some confusion.

Maybe I am wrong, but IMO in this context you are not using a NadaValue. PublicInteger and Array are the types for the future values you will assign in the runtime and PublicIntegerType and ArrayType are the type definitions something like a metatype and they only live in Python because we need to get the type properties.

@pablojhl
Copy link
Member

pablojhl commented Nov 21, 2024

I was taking a look at the PR and I'm confused. What is a NadaValue at the compiling level exactly? IMO NadaValue should exist only at the runtime level, right?

In this context a NadaValue is what you would pass to Tuple.new for instance: PublicInteger or Array. A NadaType would be PublicIntegerType or ArrayType. The two are currently mixed in the code, which creates some confusion.

Maybe I am wrong, but IMO in this context you are not using a NadaValue. PublicInteger and Array are the types for the future values you will assign in the runtime and PublicIntegerType and ArrayType are the type definitions something like a metatype and they only live in Python because we need to get the type properties.

To clarify my worries. I worry that we introduce the term NadaValue in the dsl and that provokes a dependency between the MIR model and NadaValue in the future. In the end, we are introducing a soft dependency between the DSL and NadaValue. I would like to avoid any problems because of this in the future.

@Jmgr Jmgr force-pushed the fix/nada-type-nada-value-split branch from e9c6324 to 6ff849b Compare November 21, 2024 09:33
@Jmgr
Copy link
Contributor Author

Jmgr commented Nov 21, 2024

We can name them FutureNadaValue maybe, but it's a bit long. Yes we don't have the numerical value yet, but they still represent a value for me, compared to a type. In any case this is a draft and a WIP, so we might change the naming before putting the PR into review.

@Jmgr Jmgr force-pushed the fix/nada-type-nada-value-split branch from 6ff849b to e2635fa Compare November 21, 2024 22:34
@Jmgr
Copy link
Contributor Author

Jmgr commented Nov 22, 2024

https://github.com/NillionNetwork/nillion/pull/2403 will need to be merged first.



def nada_fn(fn, args_ty=None, return_ty=None) -> NadaFunction[T, R]:
def create_nada_fn(fn, args_ty) -> NadaFunction[T, R]:

Choose a reason for hiding this comment

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

This doesn't stop me from using this as a decorator, @create_nada_fn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a way to prevent that?

@Jmgr Jmgr force-pushed the fix/nada-type-nada-value-split branch 3 times, most recently from b239c72 to 354c025 Compare November 26, 2024 13:47
@Jmgr Jmgr force-pushed the fix/nada-type-nada-value-split branch 2 times, most recently from 6244e7e to 37b6ba5 Compare November 26, 2024 14:50
@Jmgr Jmgr force-pushed the fix/nada-type-nada-value-split branch from 37b6ba5 to 87a1965 Compare November 27, 2024 16:36
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.

4 participants