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

GH-37209: [CI][Docs][MATLAB] Remove support for MATLAB_ARROW_INTERFACE flag from CMake build system and build new MATLAB Interface code by default #37211

Merged
merged 2 commits into from
Aug 17, 2023

Conversation

kevingurney
Copy link
Member

@kevingurney kevingurney commented Aug 16, 2023

Rationale for this change

Now that the old Feather V1 code and associated build infrastructure has been removed (#37204), it makes sense to start building the new, experimental MATLAB Interface code by default (without needing to explicitly specify -D MATLAB_ARROW_INTERFACE=ON).

This pull request removes the MATLAB_ARROW_INTERFACE flag entirely, since setting it to OFF when we are building the MATLAB Interface code by default would essentially imply that no code should be built.

What changes are included in this PR?

  1. Removed mention of MATLAB_ARROW_INTERFACE flag from MATLAB README.md.
  2. Removed conditional check for MATLAB_ARROW_INTERFACE flag from MATLAB CMakeLists.txt.
  3. Removed MATLAB_ARROW_INTERFACE flag from matlab_build.sh CI script.

Are these changes tested?

Yes.

The MATLAB Interface is building as expected by default on my Debian 11 machine.

Are there any user-facing changes?

Yes.

  1. The experimental MATLAB Interface APIs will now be built by default without users explicitly specifying -D MATLAB_ARROW_INTERFACE=ON to cmake.
  2. If users specify a value for MATLAB_ARROW_INTERFACE, the value will be ignored by the CMake build system.

@github-actions
Copy link

⚠️ GitHub issue #37209 has been automatically assigned in GitHub to PR creator.

Copy link
Member

@sgilmore10 sgilmore10 left a comment

Choose a reason for hiding this comment

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

This looks good to me.

@kevingurney kevingurney changed the title GH-37209: [MATLAB] Remove support for MATLAB_ARROW_INTERFACE flag from CMake build system and build new MATLAB Interface code by default GH-37209: [CI][MATLAB] Remove support for MATLAB_ARROW_INTERFACE flag from CMake build system and build new MATLAB Interface code by default Aug 16, 2023
@kevingurney kevingurney changed the title GH-37209: [CI][MATLAB] Remove support for MATLAB_ARROW_INTERFACE flag from CMake build system and build new MATLAB Interface code by default GH-37209: [CI][Docs][MATLAB] Remove support for MATLAB_ARROW_INTERFACE flag from CMake build system and build new MATLAB Interface code by default Aug 16, 2023
@kevingurney
Copy link
Member Author

FYI - @CurtHagenlocher / @westonpace - it looks like the "Dev / Source Release and Merge Script" CI workflow is failing. The logs mention C# and FlatBuffers. Could this be related to #35699?

@westonpace
Copy link
Member

FYI - @CurtHagenlocher / @westonpace - it looks like the "Dev / Source Release and Merge Script" CI workflow is failing. The logs mention C# and FlatBuffers. Could this be related to #35699?

Seems likely. I'll take a look.

@westonpace
Copy link
Member

My mistake, I merged an old PR without thinking that I should probably rebase it first. Fix incoming in #37214

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

@kou kou merged commit 4680ca5 into apache:main Aug 17, 2023
@kou kou removed the awaiting committer review Awaiting committer review label Aug 17, 2023
@github-actions github-actions bot added the awaiting merge Awaiting merge label Aug 17, 2023
kevingurney added a commit that referenced this pull request Aug 17, 2023
… for new MATLAB APIs (e.g. `RecordBatch`, `Field`, `Schema`, etc.) (#37215)

### Rationale for this change

Over the last few months, a number of new user-facing APIs have been added or changed in the MATLAB Interface.

## Examples:

### Construction Functions

- `arrow.array`
- `arrow.field`
- `arrow.schema`
- `arrow.recordbatch`
- `arrow.boolean`, `arrow.string`, `arrow.timestamp`, etc. (type construction functions)

### Classes

- `arrow.type.Field`
- `arrow.tabular.Schema`
- `arrow.tabular.RecordBatch`
- `arrow.array.StringArray`
- `arrow.array.TimestampArray`

### Indexing Methods

- `arrow.tabular.Schema.field(i)`
- `arrow.tabular.RecordBatch.column(i)`

### Static Construction Methods

- `arrow.tabular.RecordBatch.fromArrays(a1, ..., aN)`
- `arrow.array.StringArray.fromMATLAB(a)`

---

This pull request updates the [`README.md` for the MATLAB Interface](https://github.com/apache/arrow/blob/main/matlab/README.md) to reflect these changes.

### What changes are included in this PR?

Updated MATLAB `README.md`:

1. Updated **Status** section to mention support for new Arrow types and functionality.
2. Added usage examples for `Type, `Field`, `Schema`, and `RecordBatch`.
3. Updated Feather v1 usage examples to illustrate support for types like `String` and `Boolean`.

### Are these changes tested?

N/A.

This is purely a documentation change. I manually reviewed the visual rendering of the `README.md` Markdown on GitHub.

### Are there any user-facing changes?

Yes.

1. The MATLAB `README.md` now includes more usage examples which illustrate how to use the MATLAB interface.

### Future Directions

1. Add comprehensive documentation for all classes and methods, including supported input argument types and name-value pairs.
2. Add more detailed information on future development plans and development status to the `README.md`.
3. Add a table of contents to the MATLAB `README.md`.
4. Keep the `README.md` up to date more proactively.

### Notes

1. Thank you @ sgilmore10 for your help with this pull request!
2. My apologies in advance if this is too much documentation content to review at once. We will focus on being more incremental about keeping the MATLAB documentation up to date in the future.
3. #37211 also involves updating the MATLAB `README.md`, so there is a chance we may need to rebase before merging these changes in.
* Closes: #37210

Lead-authored-by: Kevin Gurney <[email protected]>
Co-authored-by: Kevin Gurney <[email protected]>
Co-authored-by: Sutou Kouhei <[email protected]>
Co-authored-by: Sarah Gilmore <[email protected]>
Signed-off-by: Kevin Gurney <[email protected]>
@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 4680ca5.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.

@kevingurney kevingurney deleted the GH-37209 branch August 21, 2023 18:13
loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
…NTERFACE` flag from CMake build system and build new MATLAB Interface code by default (apache#37211)

### Rationale for this change

Now that the old Feather V1 code and associated build infrastructure has been removed (apache#37204), it makes sense to start building the new, experimental MATLAB Interface code by default (without needing to [explicitly specify `-D MATLAB_ARROW_INTERFACE=ON`](https://github.com/apache/arrow/tree/main/matlab#build)).

This pull request removes the `MATLAB_ARROW_INTERFACE` flag entirely, since setting it to `OFF` when we are building the MATLAB Interface code by default would essentially imply that no code should be built.

### What changes are included in this PR?

1. Removed mention of `MATLAB_ARROW_INTERFACE` flag from MATLAB `README.md`.
2. Removed conditional check for `MATLAB_ARROW_INTERFACE` flag from MATLAB `CMakeLists.txt`.
3. Removed `MATLAB_ARROW_INTERFACE` flag from `matlab_build.sh` CI script.

### Are these changes tested?

Yes.

The MATLAB Interface is building as expected by default on my Debian 11 machine.

### Are there any user-facing changes?

Yes.

1. The experimental MATLAB Interface APIs will now be built by default without users explicitly specifying `-D MATLAB_ARROW_INTERFACE=ON` to `cmake`.
2. If users specify a value for `MATLAB_ARROW_INTERFACE`, the value will be ignored by the CMake build system.
* Closes: apache#37209

Authored-by: Kevin Gurney <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
…upport for new MATLAB APIs (e.g. `RecordBatch`, `Field`, `Schema`, etc.) (apache#37215)

### Rationale for this change

Over the last few months, a number of new user-facing APIs have been added or changed in the MATLAB Interface.

## Examples:

### Construction Functions

- `arrow.array`
- `arrow.field`
- `arrow.schema`
- `arrow.recordbatch`
- `arrow.boolean`, `arrow.string`, `arrow.timestamp`, etc. (type construction functions)

### Classes

- `arrow.type.Field`
- `arrow.tabular.Schema`
- `arrow.tabular.RecordBatch`
- `arrow.array.StringArray`
- `arrow.array.TimestampArray`

### Indexing Methods

- `arrow.tabular.Schema.field(i)`
- `arrow.tabular.RecordBatch.column(i)`

### Static Construction Methods

- `arrow.tabular.RecordBatch.fromArrays(a1, ..., aN)`
- `arrow.array.StringArray.fromMATLAB(a)`

---

This pull request updates the [`README.md` for the MATLAB Interface](https://github.com/apache/arrow/blob/main/matlab/README.md) to reflect these changes.

### What changes are included in this PR?

Updated MATLAB `README.md`:

1. Updated **Status** section to mention support for new Arrow types and functionality.
2. Added usage examples for `Type, `Field`, `Schema`, and `RecordBatch`.
3. Updated Feather v1 usage examples to illustrate support for types like `String` and `Boolean`.

### Are these changes tested?

N/A.

This is purely a documentation change. I manually reviewed the visual rendering of the `README.md` Markdown on GitHub.

### Are there any user-facing changes?

Yes.

1. The MATLAB `README.md` now includes more usage examples which illustrate how to use the MATLAB interface.

### Future Directions

1. Add comprehensive documentation for all classes and methods, including supported input argument types and name-value pairs.
2. Add more detailed information on future development plans and development status to the `README.md`.
3. Add a table of contents to the MATLAB `README.md`.
4. Keep the `README.md` up to date more proactively.

### Notes

1. Thank you @ sgilmore10 for your help with this pull request!
2. My apologies in advance if this is too much documentation content to review at once. We will focus on being more incremental about keeping the MATLAB documentation up to date in the future.
3. apache#37211 also involves updating the MATLAB `README.md`, so there is a chance we may need to rebase before merging these changes in.
* Closes: apache#37210

Lead-authored-by: Kevin Gurney <[email protected]>
Co-authored-by: Kevin Gurney <[email protected]>
Co-authored-by: Sutou Kouhei <[email protected]>
Co-authored-by: Sarah Gilmore <[email protected]>
Signed-off-by: Kevin Gurney <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants