Skip to content
This repository was archived by the owner on May 9, 2024. It is now read-only.

[README] Update readme build info. #166

Merged
merged 1 commit into from
Feb 1, 2023
Merged

Conversation

Devjiu
Copy link
Contributor

@Devjiu Devjiu commented Jan 18, 2023

This commit updates build instructions and removes obsolete information.

Signed-off-by: Dmitrii Makarenko [email protected]

README.md Outdated
make install
```bash
mkdir build && cd build
cmake -DENABLE_CUDA=off ..
Copy link
Contributor

Choose a reason for hiding this comment

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

Heterogeneous is a part of our library name but build instructions are for the CPU-only version. Should we propose CUDA enabled build as the default option and describe CPU-only version as an alternative for cases when the CUDA toolkit is not available?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, will update CUDA enabled instruction when figure out how to compile it with modin.

README.md Outdated

## Test

Python tests can be run from the python source directory using `pytest`.

```bash
pytest python/tests/
Copy link
Contributor

Choose a reason for hiding this comment

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

You mentioned it fails due to the missing Modin dependency. If we are fixing the documentation, then it would be nice to include all the required steps to successfully run tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@Devjiu Devjiu force-pushed the dmitriim/update-readme branch from c21eeb3 to da6411c Compare January 19, 2023 15:53
@Devjiu Devjiu requested a review from kurapov-peter January 19, 2023 15:56
@Devjiu
Copy link
Contributor Author

Devjiu commented Jan 19, 2023

Looks like tests are not stable - Build / sanity-test / build (push) Failing after 50m.

Also, I would suggest skipping a full rebuild for such minor changes.

@Devjiu Devjiu force-pushed the dmitriim/update-readme branch from da6411c to aa4eff9 Compare January 20, 2023 18:22
@Devjiu Devjiu self-assigned this Jan 20, 2023
@Devjiu Devjiu added the documentation Improvements or additions to documentation label Jan 20, 2023
@Devjiu Devjiu force-pushed the dmitriim/update-readme branch from aa4eff9 to b1cc789 Compare January 30, 2023 18:43
Copy link
Contributor

@kurapov-peter kurapov-peter left a comment

Choose a reason for hiding this comment

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

Looks fine, just one comment

README.md Outdated
```

### Issues

If you meet issues during build refer to [`.github/workflows/build.yml`](https://github.com/intel-ai/hdk/blob/b0bc9ca3f599f648bcc1a8e1eb49c0f02e1f7aca/.github/workflows/build.yml). This file describes compilation steps used for CI build.
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 a relative link to the file would be more stable here so that you don't need to update it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed.

@Devjiu Devjiu force-pushed the dmitriim/update-readme branch from b1cc789 to c5a8e84 Compare January 31, 2023 17:05
README.md Outdated

In `setup_class(..)` body.

Logs are by default located at `mapd_log/` folder.
Copy link
Contributor

Choose a reason for hiding this comment

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

@kurapov-peter @alexbaden @leshikus Do we have any scripts working with logs or depending on the log dir in some other way? It would be nice to finally rename this dir.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we do except for CI log uploading maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated according to #180

This commit updates build instructions and removes obsolete information.

Signed-off-by: Dmitrii Makarenko <[email protected]>
@Devjiu Devjiu force-pushed the dmitriim/update-readme branch from c5a8e84 to 7842966 Compare February 1, 2023 10:59
@kurapov-peter kurapov-peter merged commit 4884b72 into main Feb 1, 2023
@kurapov-peter kurapov-peter deleted the dmitriim/update-readme branch February 1, 2023 14:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants