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

ARROW-10627: [Rust] Loosen cfg restrictions for wasm32 #8798

Closed
wants to merge 1 commit into from

Conversation

nevi-me
Copy link
Contributor

@nevi-me nevi-me commented Nov 29, 2020

One of our recent changes made compiling under non-x86/amd64 platforms to return errors.
This removes those restrictions as a quick fix.

I'm a bit wary of introducing wasm32 targets into CI for now, as the tests don't yet work, and we would just increase CI time for everyone.
There is also a separate JIRA for this work, so I'm not including it here.

One of our recent changes made compiling under non-x86/amd64 platforms to return errors.
This removes those restrictions as a quick fix.

I'm a bit wary of introducing `wasm32` targets into CI for now, as the tests don't yet work.
There is also a separate JIRA for this work, so I'm not including it here.
@github-actions
Copy link

Copy link
Member

@jorgecarleitao jorgecarleitao left a comment

Choose a reason for hiding this comment

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

LGTM.

We could place it as a cron job. The issue is that those do not report anywhere AFAIK :(

@nevi-me
Copy link
Contributor Author

nevi-me commented Nov 29, 2020

The issue is that those do not report anywhere AFAIK :(

I thought of that, but the lack of reporting other than an ignoreable e-mail is a deterrent.
There's a stale PR trying to implement wasm32 support. I intend on picking it up at some point, so I'm delegating the CI work to it.

@jorgecarleitao
Copy link
Member

IMO we should refactor the CI: each compilation should be a different job with a diffetent cache so that everything can be done in parallel.

@nevi-me
Copy link
Contributor Author

nevi-me commented Nov 29, 2020

Different job per crate, or per target? (stable, various features with nightly, non-x86 targets)

@nevi-me nevi-me changed the title ARROW-10267: [Rust] Loosen cfg restrictions for wasm32 ARROW-10627: [Rust] Loosen cfg restrictions for wasm32 Nov 29, 2020
@github-actions
Copy link

@jorgecarleitao
Copy link
Member

jorgecarleitao commented Nov 29, 2020

(this is not for now and not blocking this PR, it is just a thought)

Not necessarily per target or crate:

As I see it, we have builds that we do sequentially on the script.sh, but that do not benefit from being built in sequence. Even when they do not benefit, I think that we would benefit from splitting them in multiple jobs and use artifacts to share the common state (target in this case) between them, so that caching is more granular.

More generally, our builds are basically a DAG where each node is an execution that benefits from having artifacts available:

  • build arrow ext dependencies libs: []
  • build arrow lib: [arrow ext dependencies libs]
  • build parquet lib: [parquet ext dependencies libs, arrow lib]
  • build arrow tests bin: [arrow tests ext dependencies, arrow lib]
  • build parquet tests bin: [arrow tests ext dependencies, arrow lib, parquet]
  • run tests: [arrow tests bin]

A feature flag is a new build of the lib+bin, but typically shares the same external dependencies and thus would be something like

  • build arrow lib simd: [arrow ext dependencies libs]
  • build arrow tests bin: [arrow tests ext dependencies, arrow lib simd]
  • run tests simd: [arrow tests bin]

Each tuple (architecture, compiler, compiler flags) does not share lineage with other tuples and can run in parallel.

Currently we run our DAG in sequence. However, there are many nodes on this DAG that do not depend on each other and can run in parallel (different jobs in github flow).

IMO if we split our build in different jobs that outputs an artifact and create a DAG of these jobs, we are able to run our pipeline faster by leveraging parallelism of the build.

This is something that I fielded on the mailing list in the context of the integration tests, but that it is also applicable to our own builds.

@nevi-me
Copy link
Contributor Author

nevi-me commented Nov 29, 2020

I don't have much experience with Github Actions, but I know that Gitlab CI allows one to express that DAG structure very well; so I understand what you mean.

@andygrove andygrove closed this in a302657 Dec 1, 2020
@nevi-me nevi-me deleted the ARROW-10627 branch January 5, 2021 19:12
GeorgeAp pushed a commit to sirensolutions/arrow that referenced this pull request Jun 7, 2021
One of our recent changes made compiling under non-x86/amd64 platforms to return errors.
This removes those restrictions as a quick fix.

I'm a bit wary of introducing `wasm32` targets into CI for now, as the tests don't yet work, and we would just increase CI time for everyone.
There is also a separate JIRA for this work, so I'm not including it here.

Closes apache#8798 from nevi-me/ARROW-10627

Authored-by: Neville Dipale <nevilledips@gmail.com>
Signed-off-by: Andy Grove <andygrove73@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants