rust: create multi-file run loader

[wchargin]

Summary:
This patch adds a RunLoader that reads from multiple event files
within a single run. For now, it only keeps track of scalars, and it
doesn’t do anything with the data that it reads. But it persists
everything that we’ll need for the rest of the pipeline.

Test Plan:
Unit tests included: they create a test log directory and write some
event files, then ensure that they can be read back properly.

wchargin-branch: rust-run-loader

[wchargin]

This patch is against the combined diffbase of #4340, #4341, and #4342.
Please ignore diffbase commits. (Currently, the only commit under review
is 997ddfa.) Thanks!

edit: Two of these have merged, so I’ve linearized.

[wchargin]

A couple tour-guide comments…

[wchargin]

On this line, dyn std::error::Error opts us into dynamic dispatch: the
error may be any value implementing Error, along with its vtable.
This isn’t useful for programmatic callers, since it’s hard to usefully
inspect the cause, but it’s really convenient for main functions or test
functions, like this one.

[wchargin]

Of some interest: you have probably seen us write .collect() before to
turn an iterator into a vector. But here we’re turning one into a hash
set! In fact, collect can generate any collection type, as long as
it implements FromIterator. The specific type in any instance is
determined by context, and the compiler will complain if there’s not
enough information to uniquely identify a type. This is sometimes called
target typing.

[stephanwlee]

just confirming an obvious. what happens to the old reference to v? Does compiler essentially deallocates it since the owner disavowed it?

[wchargin]

Yep. We say that the old value is dropped here, and so the compiler
will call the appropriate destructors and free the associated memory.

[stephanwlee]

Is it kosher to "import" mid module? If so, when is this pattern used? I suspect that a good compiler will do the right thing no matter how you write this but I am still curious as to when I should prefer this.

[wchargin]

Yeah, this is fine!

First, note that Rust imports are always pure. Modules do not have side
effects. So you can freely reorder imports (rustfmt sorts them) or
declare them at any scope that you like without worrying about bugs.

So, then, it’s all about scope. The general Rust convention is to import
most types and traits that you use at top level, and modules if you
want, too. Importing bare functions is rarer, I think, but not unheard
of. I like to be able to see data_compat::initial_metadata(...) to
clarify that it’s not just a helper function in this module.

So I could have imported btree_map::Entry at top level. But I think
that that would be a wee bit confusing, because if you see Entry in
some arbitrary place in the code, it’s not obvious what that refers to
(as opposed to, e.g., HashMap).

I also could have imported btree_map at top level, and that would also
have been reasonable imho. I don’t really have strong opinions here.

Some examples of my conventions:

// OK: types and traits that are clear from context
use std::collections::{HashMap, HashSet};
use std::io::{Read, Write};

// OK: modules, aliases
use std::io;
use std::path;
use crate::proto::tensorboard as pb;

// Avoid: types not clear from context
// use std::io::Error;  // a fn returns "Error"---what kind of error?

// Weak-avoid: standalone functions
// use std::iter::once;  // seeing "iter::once" would be clearer

[stephanwlee]

No action required.

Interesting; I thought the module would consider an inevitable abstraction of the file system and operate on testable EventFile or something akin to that.

I assume this is actually writing these events out to the temp directory (at sync_all call in L315)?

[wchargin]

That was certainly my plan, to parameterize the reader:

pub struct RunLoader<R> {
    files: HashMap<PathBuf, EventFile<R>>,
    // ...
}
impl<R: Read> for RunLoader { /* ... */ }

This works swimmingly for reading, but the problem is that Read is
just a stream type, and doesn’t actually give you a way to open a
file. So I would have needed to define something like:

trait Filesystem {
    type File: Read + Seek;
    fn open(p: &Path) -> io::Result<Self::File>;
}

struct RealFilesystem;
impl Filesystem for RealFilesystem {
    type File = std::fs::File;
    fn open(p: &Path) -> io::Result<Self::File> { std::fs::File::open(p) }
}

And this works perfectly fine, and we probably will want it
eventually, but I didn’t really want to bother implementing my own fake
in-memory file system when the real filesystem will do just fine.

I assume this is actually writing these events out to the temp
directory (at sync_all call in L315)?

Yep. The sync_all is an fsync(2). Without it, the files would still
be written when they’re dropped, but here we needed to keep a reference
to them later, so this flushes the BufWriter’s buffer.

[stephanwlee]

<_>! One day I will see a code with my favorite emoji, >_<

[wchargin]

Hmm… I’m having trouble thinking of a piece of standard Rust syntax that
contains >_<, since _ is not a valid identifier by itself. :-(
But this time, macros can save the day:

macro_rules! swlmoji {
    (<_>) => { "meep" };
    (>_<) => { "mrrp" };
}
fn main() {
    println!("{}", swlmoji!(<_>));
    println!("{}", swlmoji!(>_<));
}

[wchargin]

(oh, and the _ in Vec<_> means “please infer type parameter” :-) )

[stephanwlee]

Testing my understanding: I assume the loader.time_series does not actually sort/order events by step, right? It is just happens to be sorted since we wrote as such in L311-L314. Don't think I've read the reservoir module thoroughly to know that on top of my head, but it looks like rsv.offer simply pushes onto the vec.

[wchargin]

It is sorted because the reservoir preempts any non-preceding records
before pushing onto the vector, so when it pushes, the step of the
new record is always larger than the steps of all other records.

Including a preemption in this test is probably a pretty reasonable
idea. I’ll go ahead and do that.