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

Add arm64 atomics fix script #1788

Merged
merged 2 commits into from
May 15, 2023
Merged

Add arm64 atomics fix script #1788

merged 2 commits into from
May 15, 2023

Conversation

panpilkarz
Copy link
Contributor

@panpilkarz panpilkarz commented May 9, 2023

Add install command to Makefile, which consolidates following:

yarn install
poetry install
ape compile

and conditionally fixes atomics problem on arm64 chips.

@panpilkarz panpilkarz requested review from istankovic, fredo and a team May 9, 2023 12:54

TARGET="$(poetry env info -p)/lib/python*/site-packages/"

pip install --upgrade atomics --platform=universal2 --no-deps --target $TARGET
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, I feel like this should be handled by poetry.
In other words, you shouldn't need to upgrade atomics this way, but you would perhaps just need to specify --platform to poetry when doing the install. Doesn't something like that work?

Copy link
Contributor

Choose a reason for hiding this comment

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

poetry add at least has this:

--platform=PLATFORM    Platforms for which the dependency must be installed.

so at least it looks like it's possibe to have multiple platforms supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't find a way how to achieve that in poetry, but while doing more research, figured out a simpler command:

arch -arm64 pip install atomics --no-binary :all:

Would it be OK to just add it as a note to https://docs.beamerbridge.com/contributing.html?

Or perhaps add an install script, which would execute poetry install inside and conditionally execute above pip command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess having install script wrapping up yarn install and poetry install and ape compile and fixing arm64 issue would be a nice shortcut.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess having install script wrapping up yarn install and poetry install and ape compile and fixing arm64 issue would be a nice shortcut.

Do yarn and ape need that kind of wrapper, though?
From what I understand, you only had issues with the atomics package -- everything else worked?

Personally, I'm leaning more toward a Mac-specific note in the docs, but it would also be nice to have feedback from people actually using Arm Macs. @fredo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant following script (pseudo code):

yarn install
poetry install
if arm64 {
   arch -arm64 pip install atomics --no-binary :all:
}
poetry run ape compile

Then to shorten the docs with running make install instead of 3 separate commands.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think here are two different features:

One is to improve the handling of the missing arm64 build wheel in atomics, and second creating a make install command which also handles yarn and compilation of contracts.

In general I'm in favor of shorter commands to get started. However, I see that the main benefit here would be the fix for atomics.
@panpilkarz Is there a particular reason why you also added the compile command into the install target?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No real reason. Happy to get rid of it.

@panpilkarz panpilkarz force-pushed the arm64-fix-script branch 2 times, most recently from 3dbb87a to 2817e44 Compare May 9, 2023 17:15
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Outdated

CODE_DIRS = beamer/ scripts/
CONTRACTS = "contracts/**/*.sol"
IMAGE_NAME := beamer
ARCH := $(shell arch)
Copy link
Contributor

Choose a reason for hiding this comment

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

This results in

make: arch: No such file or directory

on my machine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please check now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just use uname -m since that is available everywhere.

@panpilkarz
Copy link
Contributor Author

panpilkarz commented May 10, 2023

@istankovic to follow up your concern raised on chat:

could you please take a look at python-poetry/poetry#2138 (comment) ?

I think it sucks to use a fixed url with fixed version of the package.

In my solution, "the correct" version of package will be reinstalled, the one which poetry figured out after resolving dependencies puzzle. To achieve that, after poetry install is done, the code checks for atomics version installed, and reinstalls the same version from universal2 binary.

@panpilkarz
Copy link
Contributor Author

@istankovic Also stuff like this:

torch = [
   {markers = "sys_platform == 'linux'", url = "https://download.pytorch.org/whl/cpu/torch-1.8.1%2Bcpu-cp38-cp38-linux_x86_64.whl"},
   {markers = "sys_platform == 'darwin'", url = "https://download.pytorch.org/whl/cpu/torch-1.8.1-cp38-none-macosx_10_9_x86_64.whl"}
]

makes it possible to pin a version for a platform not for architecture. No need to patch for darwin users with Intel.

@istankovic
Copy link
Contributor

I think it sucks to use a fixed url with fixed version of the package.

Yeah, I completely agree. Poetry still doesn't have a nice answer for this....

@istankovic
Copy link
Contributor

Could you please squash the commits so that we end up with one commit changing only the Makefile, and another for the docs?

@panpilkarz panpilkarz force-pushed the arm64-fix-script branch 2 times, most recently from 742ee8a to a0a347b Compare May 12, 2023 09:27
Makefile Outdated
yarn install
poetry install
ifeq ($(ARCH),arm64)
$(eval ATOMICS := $(shell poetry export | egrep 'atomics==' | awk '{print $$1}'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: egrep is deprecated (not sure about the version you have on Mac, though)

egrep: warning: egrep is obsolescent; using grep -E

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right. used grep -E in difflint work for that reason.

Copy link
Contributor

@istankovic istankovic left a comment

Choose a reason for hiding this comment

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

Looks good!

@panpilkarz panpilkarz force-pushed the arm64-fix-script branch 2 times, most recently from 1c07757 to 713f34a Compare May 15, 2023 06:27
@fredo fredo merged commit 762479f into main May 15, 2023
@istankovic istankovic deleted the arm64-fix-script branch September 12, 2023 12:49
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.

3 participants