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

feat: vyper 0.4 support #110

Merged
merged 15 commits into from
Jun 12, 2024
Merged

feat: vyper 0.4 support #110

merged 15 commits into from
Jun 12, 2024

Conversation

antazoey
Copy link
Member

What I did

Make it so can compile on 0.4.0

How I did it

Had to use absolute paths.

How to verify it

Checklist

  • Passes all linting checks (pre-commit and CI jobs)
  • New test cases have been added and are passing
  • Documentation has been updated
  • PR title follows Conventional Commit standard (will be automatically included in the changelog)

Copy link
Member

@fubuloubu fubuloubu 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 checked against 0.3.10 and 0.4.0?

setup.py Outdated Show resolved Hide resolved
@antazoey antazoey marked this pull request as draft April 11, 2024 15:07
@antazoey
Copy link
Member Author

Is this checked against 0.3.10 and 0.4.0?

Sorry meant to make this a draft.. The tests are all pre-0.4.0, but I need to add more tests for 0.4.0 range.

@charles-cooper
Copy link

i think rather than using absolute paths, you can add the "./" to the search path

@antazoey
Copy link
Member Author

i think rather than using absolute paths, you can add the "./" to the search path

I tried this but was running into other issues, I can keep trying though but I was in a tangled web of confusion.

@antazoey
Copy link
Member Author

Looks like vyper in the 0.3.0 range does not like the absolute paths

@pcaversaccio
Copy link

currently it fails to process .vyi files
image

@pcaversaccio
Copy link

Looks like imports don't resolve yet:

image

@antazoey
Copy link
Member Author

WIP - still in development, please be patient.

fubuloubu
fubuloubu previously approved these changes Apr 11, 2024
ape_vyper/_utils.py Outdated Show resolved Hide resolved
ape_vyper/__init__.py Outdated Show resolved Hide resolved
ape_vyper/_utils.py Outdated Show resolved Hide resolved
fubuloubu
fubuloubu previously approved these changes Apr 12, 2024
@pcaversaccio
Copy link

pcaversaccio commented Apr 13, 2024

@charles-cooper
Copy link

i think constructing the input dict from just a directory is going to be pretty complicated, because it's hard to predict how the compiler will interact with search path (cf. for instance: vyperlang/vyper@b0ea5b6, vyperlang/vyper@d372378, vyperlang/vyper@c6f457a, vyperlang/vyper@a3bc3eb, vyperlang/vyper@5d10ea0).

i'm not sure what direction ape wants to take here, but maybe the best thing might be to call the compiler and recursively get its ast output (per vyperlang/vyper@9428196), or just use the cli and get -f combined_json or something. we could do something else too like adding -f import_graph as a compiler output option or something, i'm open to suggestions.

@fubuloubu
Copy link
Member

we could do something else too like adding -f import_graph as a compiler output option or something, i'm open to suggestions.

Having the import graph would be extremely useful, in both directions (know which code modules import which other code modules so that a downstream module change will trigger us to recompile one that hasn't because of the dependency)

@charles-cooper
Copy link

charles-cooper commented Apr 14, 2024

@fubuloubu also check -f annotated_ast output, it contains information about the imported modules including the sha256sum of the content. you can use this to compute a reproducible import graph. vyperlang/vyper#3891 goes further and includes an integrity hash output, which is the recursive hash of the file and all its dependencies. in vyperlang/vyper#3891 we could maybe add a json output, i'm not sure if that's very helpful here (since in any case we have to run in "regular" non-json mode first to grab the import graph).

maybe the best thing is to ditch standard-json input here? because it's not trivial to construct. from CLI, everything just works since the compiler will resolve things in the filesystem automatically.

if you want to keep using standard-json and one-shot the compilation (i.e., not have to query the compiler multiple times about a file), the other way would be to bundle everything ape knows is in the environment (hypothetical example: shove contracts/, libraries/, modules/) all into json input. that provides the best chance of successful compilation, although you still might end up getting divergences between what the compiler thinks is in the environment and what ape thinks is in the environment.

@antazoey
Copy link
Member Author

@pcaversaccio I promise I will make sure snekmate compiles before we merge this.

@fubuloubu
Copy link
Member

fubuloubu commented Apr 15, 2024

@fubuloubu also check -f annotated_ast output, it contains information about the imported modules including the sha256sum of the content. you can use this to compute a reproducible import graph. vyperlang/vyper#3891 goes further and includes an integrity hash output, which is the recursive hash of the file and all its dependencies. in vyperlang/vyper#3891 we could maybe add a json output, i'm not sure if that's very helpful here (since in any case we have to run in "regular" non-json mode first to grab the import graph).

maybe the best thing is to ditch standard-json input here? because it's not trivial to construct. from CLI, everything just works since the compiler will resolve things in the filesystem automatically.

if you want to keep using standard-json and one-shot the compilation (i.e., not have to query the compiler multiple times about a file), the other way would be to bundle everything ape knows is in the environment (hypothetical example: shove contracts/, libraries/, modules/) all into json input. that provides the best chance of successful compilation, although you still might end up getting divergences between what the compiler thinks is in the environment and what ape thinks is in the environment.

We basically want a way to know that we don't need to even call vyper in the first place because none of the files (or their dependencies) have made an update which should change their output.

Since subprocessing out to vyper is more expensive than not, having the checksum information in Ape's project metadata helps us avoid unnecessary extra compiler invocations

It is also still valuable that vyper does this check to, but would be great if that information were made accessible back to how Ape stores it's own project cache (which may include multiple compilers to check for)

@antazoey
Copy link
Member Author

Let's see if this is even a problem first as well. This is still in development, but getting closer each day. If we can handle Solidity, I think we can handle Vyper.

@fubuloubu
Copy link
Member

Let's see if this is even a problem first as well. This is still in development, but getting closer each day. If we can handle Solidity, I think we can handle Vyper.

0.4.0 made a big shift to absolute file locations from std json input, this is very different than how solidity or vyper (in the past) has handled file inputs

but yeah, calling annotated_ast (if available) sounds like it will give us the checksum information we can store in our own metadata cache. I missed the integrity hash concept, but since we only add back references for dependencies to be able to determine this, it could also help. That was non-standard in our EthPM v3 implementation

@charles-cooper
Copy link

charles-cooper commented Apr 15, 2024

We basically want a way to know that we don't need to even call vyper in the first place because none of the files (or their dependencies) have made an update which should change their output.

Since subprocessing out to vyper is more expensive than not, having the checksum information in Ape's project metadata helps us avoid unnecessary extra compiler invocations

since dependencies can change even if source code (of the compilation target) has not changed, there are two surefire ways to do this. one is going to be to call the vyper compiler and get the integrity hash (this only runs the frontend analysis passes, which based on latest benchmarks can process roughly 1000 lines of source code per second). one thing i can do (and i was thinking about doing this anyway) is to move the compiler's import analysis into an even earlier phase, so basically the only work the compiler does to get the integrity hash is resolving imports + hashing. but it would be a bit nontrivial to implement.

the other way is to package all the project files into a single package (like standard json input) and check that nothing in the package has changed.

@fubuloubu
Copy link
Member

one thing i can do (and i was thinking about doing this anyway) is to move the compiler's import analysis into an even earlier phase, so basically the only work the compiler does to get the integrity hash is resolving imports + hashing. but it would be a bit nontrivial to implement.

we essentially do import analysis to build the graph of the project, so this would be really nice to expose. we may in the future execute multiple compiler processes in parallel if we determine (based on the graph) that there are disjoint sets within the project (allowing us to parallelize compiler invocations). an obvious first version of this would be to just parallelize invocations for vyper and solidity files within the same project

@charles-cooper
Copy link

we essentially do import analysis to build the graph of the project, so this would be really nice to expose. we may in the future execute multiple compiler processes in parallel if we determine (based on the graph) that there are disjoint sets within the project (allowing us to parallelize compiler invocations). an obvious first version of this would be to just parallelize invocations for vyper and solidity files within the same project

there isn't really work-sharing between different compilation targets. like if X imports A, and Y imports A, each one will re-compile A from scratch. so you can just always parallelize compilation targets.

@fubuloubu
Copy link
Member

we essentially do import analysis to build the graph of the project, so this would be really nice to expose. we may in the future execute multiple compiler processes in parallel if we determine (based on the graph) that there are disjoint sets within the project (allowing us to parallelize compiler invocations). an obvious first version of this would be to just parallelize invocations for vyper and solidity files within the same project

there isn't really work-sharing between different compilation targets. like if X imports A, and Y imports A, each one will re-compile A from scratch. so you can just always parallelize compilation targets.

how do we know what the targets are?

@antazoey antazoey marked this pull request as ready for review June 10, 2024 16:06
@antazoey antazoey requested a review from fubuloubu June 10, 2024 16:06
fubuloubu
fubuloubu previously approved these changes Jun 10, 2024
Copy link
Member

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

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

Doesn't have yo be here, but some tests using modules from snekmate would be great

@charles-cooper
Copy link

we essentially do import analysis to build the graph of the project, so this would be really nice to expose. we may in the future execute multiple compiler processes in parallel if we determine (based on the graph) that there are disjoint sets within the project (allowing us to parallelize compiler invocations). an obvious first version of this would be to just parallelize invocations for vyper and solidity files within the same project

there isn't really work-sharing between different compilation targets. like if X imports A, and Y imports A, each one will re-compile A from scratch. so you can just always parallelize compilation targets.

how do we know what the targets are?

oh i missed this -- targets are whatever the user asks for. (anything in contracts/?)

@fubuloubu
Copy link
Member

we essentially do import analysis to build the graph of the project, so this would be really nice to expose. we may in the future execute multiple compiler processes in parallel if we determine (based on the graph) that there are disjoint sets within the project (allowing us to parallelize compiler invocations). an obvious first version of this would be to just parallelize invocations for vyper and solidity files within the same project

there isn't really work-sharing between different compilation targets. like if X imports A, and Y imports A, each one will re-compile A from scratch. so you can just always parallelize compilation targets.

how do we know what the targets are?

oh i missed this -- targets are whatever the user asks for. (anything in contracts/?)

we don't require the user to manually specify their targets, typically we try to identify the most efficient way to compile everything so that when the user goes and asks for a specific target (e.g. project.MyContractTarget) that target is already compiled and available for immediate use.

requiring the user to specify all of their targets strikes me as poor developer experience.

@pcaversaccio
Copy link

So I tested the latest commit and snekmate contracts compile: pcaversaccio/snekmate#244.

Two notes:

  1. When installing Ape, Ape pulls in the old Vyper 0.3.x series (example):
Collecting vyper~=0.3.7 (from ape-vyper==0.1.0b3.dev69+g6154453)
  Downloading vyper-0.3.10-py3-none-any.whl.metadata (6.7 kB)

This is unfortunate since I need to uninstall Vyper and reinstall the new Vyper later again (otherwise I have Vyper 0.3.x installed locally or in the CI). It would be great to have an option to disable this installation step (@antazoey explained to me why this is currently needed for the flattener).

  1. @charles-cooper: in the ape-config.yaml of my PR you can see how I set contracts_folder: src/snekmate and exclude: r"(?!.*_mock\.vy$)", which can be used to isolate what is being compiled by Ape. Also, since 0.8.0, the default base path for compiling projects is now the project root instead of the contracts/ folder.

@fubuloubu
Copy link
Member

  1. When installing Ape, Ape pulls in the old Vyper 0.3.x series (example):
Collecting vyper~=0.3.7 (from ape-vyper==0.1.0b3.dev69+g6154453)
  Downloading vyper-0.3.10-py3-none-any.whl.metadata (6.7 kB)

This is unfortunate since I need to uninstall Vyper and reinstall the new Vyper later again (otherwise I have Vyper 0.3.x installed locally or in the CI). It would be great to have an option to disable this installation step (@antazoey explained to me why this is currently needed for the flattener).

We should probably only have a lower pin for the flattener CLI, or as low a pin as we can manage

Wonder if it makes sense as an extra (and don't add flattener CLI if vyper not installed?) Spitballing here

@antazoey
Copy link
Member Author

@fubuloubu

Wonder if it makes sense as an extra (and don't add flattener CLI if vyper not installed?) Spitballing here

This was my thought as well.

Another option I was going to look into was check what was being imported from vyper and see if i can copy it in manually (provided it is the same across all versions). It would save a lot of headache and the code won't change much...

Either way, can we do this in another PR?

@fubuloubu
Copy link
Member

Either way, can we do this in another PR?

sounds good, there is a workaround for now (just re-install vyper afterwards)

Another option I was going to look into was check what was being imported from vyper and see if i can copy it in manually (provided it is the same across all versions). It would save a lot of headache and the code won't change much...

can you mark this in an issue?

@antazoey
Copy link
Member Author

can you mark this in an issue?

#114

@antazoey antazoey merged commit 7096c4e into ApeWorX:main Jun 12, 2024
13 checks passed
@antazoey antazoey deleted the feat/vyper4 branch June 12, 2024 03:40
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