Skip to content

Update README.md #104

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

Merged
merged 5 commits into from
Aug 26, 2017
Merged

Update README.md #104

merged 5 commits into from
Aug 26, 2017

Conversation

alanyee
Copy link
Contributor

@alanyee alanyee commented Aug 22, 2017

-Update details
-Made explicit HTTPS calls
-Added a link to Homebrew

-Update details
-Made explicit HTTPS calls
-Added a link to Homebrew
Copy link
Contributor

@nbigaouette-eai nbigaouette-eai left a comment

Choose a reason for hiding this comment

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

LGTM

README.md Outdated
[snipsco/tensorflow-build](https://github.com/snipsco/tensorflow-build) which provides a homebrew
tap that does essentially the same.
**macOS Note**: Via [Homebrew](https://brew.sh/) in process which, you can just run
`brew install libtensorflow`.

Copy link
Contributor

Choose a reason for hiding this comment

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

We might also want to revisit the Usage section where it is mentioned that TF is built during the crate's build process.

I believe the default now is to download a pre-built (basic CPU only) library instead of full compilation. See https://github.com/tensorflow/rust/blob/f204b39/tensorflow-sys/build.rs#L44-L52.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you rewrite that section? I am unsure what to particularly add or remove.

Copy link
Contributor

Choose a reason for hiding this comment

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

The paragraph's second sentence starts with Since TensorFlow is built during this process... but this sentence is now wrong (after PR #65).

The tensorflow-sys crate's build.rs will either download a pre-built binary (the default) or compile TF if forced to by an environment variable).

The README paragraph could be adapted to mention this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it seem good to you now?

README.md Outdated

**Notice:** This project is still under active development and not guaranteed to have a
stable API. This is especially true because the underlying TensorFlow C API has not yet
been stabilized as well.

* [Documentation](https://tensorflow.github.io/rust/tensorflow/)
* [TensorFlow website](http://tensorflow.org)
* [TensorFlow website](https://tensorflow.org)
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do you know that the certificate expired on June 29?

Copy link
Contributor

Choose a reason for hiding this comment

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

It depends on your browser, but in Firefox, I get a error page about the connection not being secure. I can then click on Advanced, and it displays the certificate's expiration date.

Remove unnecessary words
Update usage
Copy link
Contributor

@adamcrume adamcrume 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, but I'll give @nbigaouette-eai a chance to respond before I merge it.

Copy link
Contributor

@nbigaouette-eai nbigaouette-eai left a comment

Choose a reason for hiding this comment

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

LGTM!

@adamcrume adamcrume merged commit 63c13c1 into tensorflow:master Aug 26, 2017
@adamcrume
Copy link
Contributor

Thanks!

ramon-garcia pushed a commit to ramon-garcia/tensorflow-rust that referenced this pull request May 20, 2023
ramon-garcia pushed a commit to ramon-garcia/tensorflow-rust that referenced this pull request May 20, 2023
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