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

Feature gate object_store and reqwest #71

Merged
merged 15 commits into from
Mar 24, 2025

Conversation

feefladder
Copy link
Contributor

As discussed in #6, only feature gate and not pulling coalesce_ranges in-tree

@feefladder feefladder mentioned this pull request Mar 21, 2025
Copy link
Member

@kylebarron kylebarron left a comment

Choose a reason for hiding this comment

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

Can you add to CI variants that test that async-tiff compiles with each combination of features?

You can see how parquet-wasm uses cargo-all-features
https://github.com/kylebarron/parquet-wasm/blob/e85322b592ab224938861895a06f4f3947a77802/Cargo.toml#L117

https://github.com/kylebarron/parquet-wasm/blob/e85322b592ab224938861895a06f4f3947a77802/.github/workflows/test.yml#L38-L41

feefladder and others added 2 commits March 22, 2025 17:58
Co-authored-by: Kyle Barron <[email protected]>
Co-authored-by: Kyle Barron <[email protected]>
@feefladder
Copy link
Contributor Author

feefladder commented Mar 22, 2025

just wanted to note: removing object_store from default deps makes tests fail, which is fine

@feefladder
Copy link
Contributor Author

it was not fine and is fixed now

src/reader.rs Outdated
Comment on lines 73 to 86
impl AsyncFileReader for std::fs::File {
fn get_bytes(&self, range: Range<u64>) -> BoxFuture<'_, AsyncTiffResult<Bytes>> {
async move {
let mut file = self.try_clone()?;
file.seek(std::io::SeekFrom::Start(range.start))?;
let len = (range.end - range.start) as usize;
let mut buf = vec![0u8; len];
file.read_exact(&mut buf)?;
let res = Bytes::copy_from_slice(&buf);
Ok(res)
}
.boxed()
}
}
Copy link
Member

Choose a reason for hiding this comment

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

We should not add this because this will perform blocking reads on the main thread.

Instead we should probably restore the tokio integration

async-tiff/src/reader.rs

Lines 70 to 91 in 6a7a2dc

// #[cfg(feature = "tokio")]
// impl<T: tokio::io::AsyncRead + tokio::io::AsyncSeek + Unpin + Debug + Send + Sync> AsyncFileReader
// for T
// {
// fn get_bytes(&self, range: Range<u64>) -> BoxFuture<'_, AsyncTiffResult<Bytes>> {
// use tokio::io::{AsyncReadExt, AsyncSeekExt};
// async move {
// self.seek(std::io::SeekFrom::Start(range.start)).await?;
// let to_read = (range.end - range.start).try_into().unwrap();
// let mut buffer = Vec::with_capacity(to_read);
// let read = self.take(to_read as u64).read_to_end(&mut buffer).await?;
// if read != to_read {
// return Err(AsyncTiffError::EndOfFile(to_read, read));
// }
// Ok(buffer.into())
// }
// .boxed()
// }
// }
which will allow reading from a tokio async file, which I think performs file IO from a thread pool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem I had was that the feature testing also wants to test with all features disabled, and I didn't exactly know how to deal with that... added a fallback in util.rs

Copy link
Contributor Author

@feefladder feefladder Mar 23, 2025

Choose a reason for hiding this comment

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

Also tokio's asyncread require &mut self, so I changed the impl to tokio::fs::File, where we can try_clone() on &self. There doesn't seem to be a TryClone trait

@feefladder
Copy link
Contributor Author

@kylebarron should be done!

@kylebarron kylebarron enabled auto-merge (squash) March 24, 2025 19:49
@kylebarron kylebarron merged commit 960e4e1 into developmentseed:main Mar 24, 2025
6 checks passed
@kylebarron
Copy link
Member

Thanks. I updated a couple things:

  • We don't need to actually run test on each feature flag. It's fine to just check that it compiles.
  • And this adds an adapter struct to work with arbitrary tokio objects so we don't need to concretely implement for a tokio file

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.

2 participants