-
Notifications
You must be signed in to change notification settings - Fork 72
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
Start building linux-aarch64
nightlies
#1144
Conversation
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## main #1144 +/- ##
==========================================
+ Coverage 81.42% 81.53% +0.11%
==========================================
Files 78 78
Lines 4474 4474
Branches 817 817
==========================================
+ Hits 3643 3648 +5
+ Misses 650 641 -9
- Partials 181 185 +4 see 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Given the long build times for the conda nightly, it might make sense to have this run at a schedule rather than a per pr basis. Also for a quick test would it be possible to upload the artifacts from these runs to github? I wanted to give it a try on an aarch64 machine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Charles! 🙏
Had a couple comments below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok sounds like we need something more like this then
Co-authored-by: jakirkham <jakirkham@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can move the setuptools-rust
version into conda_build_config.yaml
as well to reduce duplication
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit: Maybe we can group the cross-compilation changes together to make things a bit more readable?
Co-authored-by: jakirkham <jakirkham@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks Charles! 🙏
Let's see if anything else is needed based on CI
Think it would be good to update the conda-forge recipe with these changes. Could happen in the next release. We could just raise an issue to track for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Charles! 🙏
This is looking good
GHA flagged a minor parsing error. Added a suggestion below
Co-authored-by: jakirkham <jakirkham@gmail.com>
Looks like that passed (excluding gpuCI) 🎉 |
Looks like there are some issues with the ARM packages; @ayushdg was able to get this stacktrace when trying to import in an environment with In [1]: from dask_sql import Context
---------------------------------------------------------------------------
ImportError Traceback (most recent call last)
Cell In[1], line 1
----> 1 from dask_sql import Context
File ~/miniconda3/envs/dsql2/lib/python3.10/site-packages/dask_sql/__init__.py:2
1 from . import _version, config
----> 2 from .cmd import cmd_loop
3 from .context import Context
4 from .datacontainer import Statistics
File ~/miniconda3/envs/dsql2/lib/python3.10/site-packages/dask_sql/cmd.py:26
22 except ImportError: # pragma: no cover
23 # prompt_toolkit version < 2
24 from prompt_toolkit.layout.lexers import PygmentsLexer
---> 26 from dask_sql.context import Context
28 meta_command_completer = WordCompleter(
29 ["\\l", "\\d?", "\\dt", "\\df", "\\de", "\\dm", "\\conninfo", "quit"]
30 )
33 class CompatiblePromptSession:
File ~/miniconda3/envs/dsql2/lib/python3.10/site-packages/dask_sql/context.py:12
9 from dask import config as dask_config
10 from dask.base import optimize
---> 12 from dask_planner.rust import (
13 DaskSchema,
14 DaskSQLContext,
15 DaskTable,
16 DFOptimizationException,
17 DFParsingException,
18 LogicalPlan,
19 )
21 try:
22 from dask_sql.physical.utils.statistics import parquet_statistics
ImportError: /home/adattagupta/miniconda3/envs/dsql2/lib/python3.10/site-packages/dask_planner/rust.cpython-310-aarch64-linux-gnu.so: cannot allocate memory in static TLS block |
Maybe there are some clues on how to configure Rust cross-compilation in this doc Edit: There are some notes in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps the bug is due to an issue in our requirements?
Also when initially searching for similar errors to comment ( #1144 (comment) ) had found some references to missing or linking incorrectly to GNU OpenMP. When looking at the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for struggling through that one @charlesbluca and @jakirkham that one was a bear
Co-authored-by: jakirkham <jakirkham@gmail.com>
Looks good. Thanks Charles! 🙏 |
From internal conversations with RAPIDS ops, it sounds like there would be some benefit to having
linux-aarch64
nightlies available for testing; this could also be of some benefit to us in that it gives some better insights into what issues we would run into during cross-compilation (for example, working on this PR uncovered that we needed to addlibprotobuf
to our build requirements).