Skip to content

Use reqwest and Rust 2018#5

Open
krzentner wants to merge 2 commits intombillingr:masterfrom
krzentner:master
Open

Use reqwest and Rust 2018#5
krzentner wants to merge 2 commits intombillingr:masterfrom
krzentner:master

Conversation

@krzentner
Copy link

@krzentner krzentner commented Jul 13, 2019

Basically, I just made these changes so that

  1. I could build openml-rust on my machine.
  2. Future maintenance should be a little easier.

krzentner added 2 commits July 13, 2019 10:19
@krzentner
Convert from hyper to reqwest
f02367f 
This allows some of the download code to be deleted, since reqwest can
handle it. On my system, this fixes the build, since reqwest
(indirectly) uses a more recent version of rust-native-tls.
@krzentner
Update to Rust 2018
dd7368a 
This change is made using three simple steps:
 - `cargo fix --edition`
 - `cargo fmt`
 - Add `edition = "2018"` to Cargo.toml
mbillingr
mbillingr approved these changes Jul 22, 2019
View reviewed changes
Copy link
Owner

@mbillingr mbillingr left a comment

Choose a reason for hiding this comment

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

Looks great!

@mbillingr
Copy link
Owner

mbillingr commented Jul 22, 2019

closes #4

Thank you for the contribution! I highly appreciate these sensible changes.

What exactly prevented building on your machine?

I tested your changes on a slow internet connection with a clean download cache and ran into a timeout:

2019-07-22 10:39:11 INFO [openml::openml_api::web_access] Downloading https://www.openml.org/data/v1/download/18238735/Fashion-MNIST.arff
thread 'tests::apidev2' panicked at 'called Result::unwrap() on an Err value: HttpsError(Error(Io(Custom { kind: TimedOut, error: StringError("timed out") })))', src\libcore\result.rs:999:5

This might be a local issue with my company's proxy that likes to run extensive AV scans before forwarding downloads. I will test it again on a different network this evening.

Is there anything that can be done to make reqwest timeouts more tolerant?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mbillingr mbillingr mbillingr approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@krzentner @mbillingr