Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions tensorboard/data/server/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ rust_library(
"reservoir.rs",
"scripted_reader.rs",
"tf_record.rs",
"types.rs",
] + _checked_in_proto_files,
edition = "2018",
deps = [
Expand Down
1 change: 1 addition & 0 deletions tensorboard/data/server/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ pub mod event_file;
pub mod masked_crc;
pub mod reservoir;
pub mod tf_record;
pub mod types;

#[cfg(test)]
mod scripted_reader;
Expand Down
6 changes: 2 additions & 4 deletions tensorboard/data/server/reservoir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ use rand::{
};
use rand_chacha::ChaCha20Rng;

use crate::types::Step;

/// A [reservoir sampling] data structure, with support for preemption and deferred "commits" of
/// records to a separate destination for better concurrency.
///
Expand Down Expand Up @@ -98,10 +100,6 @@ pub struct StageReservoir<T, C = ChaCha20Rng> {
seen: usize,
}

/// A step associated with a record, strictly increasing over time within a record stream.
#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Copy, Clone)]
pub struct Step(pub i64);

/// A buffer of records that have been committed and not yet evicted from the reservoir.
///
/// This is a snapshot of the reservoir contents at some point in time that is periodically updated
Expand Down
110 changes: 110 additions & 0 deletions tensorboard/data/server/types.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
/* Copyright 2020 The TensorFlow Authors. All Rights Reserved.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
==============================================================================*/

//! Core simple types.

use std::borrow::Borrow;

/// A step associated with a record, strictly increasing over time within a record stream.
#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Copy, Clone)]
pub struct Step(pub i64);

/// The wall time of a TensorBoard event.
///
/// Wall times represent floating-point seconds since Unix epoch. They must be finite and non-NaN.
#[derive(Debug, PartialEq, PartialOrd, Copy, Clone)]
pub struct WallTime(f64);

impl WallTime {
/// Parses a wall time from a time stamp representing seconds since Unix epoch.
///
/// Returns `None` if the given time is infinite or NaN.
pub fn new(time: f64) -> Option<Self> {
if time.is_finite() {
Some(WallTime(time))
} else {
None
}
}
}

// Wall times are totally ordered and have a total equivalence relation, since we guarantee that
// they are not NaN.
#[allow(clippy::derive_ord_xor_partial_ord)] // okay because it agrees with `PartialOrd` impl
Copy link
Contributor

Choose a reason for hiding this comment

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

reading this, it looks like PartialOrd can never disagree with Ord. If so, what is this derive_ord_xor_partial_ord really enforcing?

After reading a bit more about this https://rust-lang.github.io/rust-clippy/master/index.html#derive_ord_xor_partial_ord, why is the impl Ord
block required when already given #[derive(PartialOrd)]?

(for my education of course)

Copy link
Contributor Author

@wchargin wchargin Nov 18, 2020

Choose a reason for hiding this comment

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

Correct: the PartialOrd and Ord implementations for a given type
must agree, else many operations are undefined. (Not the scary kind of
“undefined behavior”; it’s just a contract violation on basically every
API that cares about orderings.)

This line disables a Clippy lint, whose docs you’ve linked to. I think
that the purpose is to flag code like this:

#[derive(PartialOrd)]
struct BackwardInt(i64);

impl Ord for BackwardInt {
    fn cmp(&self, other: &Self) -> Ordering {
        // Our type is ordered backward!
        self.0.cmp(&other.0).reverse()
    }
}

This Ord implementation is fine by itself, and you might think that
#[derive(PartialOrd)] would implement PartialOrd for our type by
delegating to its Ord. But in fact it would actually delegate to the
PartialOrds of its fields, which would make the PartialOrd and Ord
inconsistent in this case.

In our case, the derived PartialOrd is consistent with the manual
Ord (which you can tell since cmp just calls partial_cmp), so I
suppressed the lint error. We could write it out—

impl PartialOrd for WallTime {
    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
        Some(self.cmp(other))
    }
}

—but I didn’t see much reason to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here’s a topical upstream request to teach the lint to recognize this
safe pattern:
rust-lang/rust-clippy#6219

impl Ord for WallTime {
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
self.partial_cmp(&other)
.unwrap_or_else(|| unreachable!("{:?} <> {:?}", &self, &other))
}
}
impl Eq for WallTime {}
Copy link
Contributor

Choose a reason for hiding this comment

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

For my education, is this any different than #[derive(Eq)]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does the same thing that derive(Eq) would do, since Eq itself has
no methods (it’s a marker interface). But you’re only permitted to
derive(Eq) on types all of whose fields are also Eq. In this case,
we have a field of type f64, which is not Eq (because of NaN), so
we cannot use derive(Eq):

$ git diff -U1
diff --git a/tensorboard/data/server/types.rs b/tensorboard/data/server/types.rs
index 59a764504..755329912 100644
--- a/tensorboard/data/server/types.rs
+++ b/tensorboard/data/server/types.rs
@@ -26,3 +26,3 @@ pub struct Step(pub i64);
 /// Wall times represent floating-point seconds since Unix epoch. They must be finite and non-NaN.
-#[derive(Debug, PartialEq, PartialOrd, Copy, Clone)]
+#[derive(Debug, PartialEq, PartialOrd, Copy, Clone, Eq)]
 pub struct WallTime(f64);
@@ -51,3 +51,2 @@ impl Ord for WallTime {
 }
-impl Eq for WallTime {}
 
$ cargo check
error[E0277]: the trait bound `f64: std::cmp::Eq` is not satisfied
   --> types.rs:28:21
    |
28  | pub struct WallTime(f64);
    |                     ^^^ the trait `std::cmp::Eq` is not implemented for `f64`
    | 

Instead, impl Eq for WallTime {} is our explicit way of declaring that
our type really does have a total equivalence relation, even though the
types of its fields do not.


impl From<WallTime> for f64 {
fn from(wt: WallTime) -> f64 {
wt.0
}
}

/// The name of a time series within the context of a run.
///
/// Tag names are valid Unicode text strings. They should be non-empty, though this type does not
/// enforce that.
#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Clone)]
pub struct Tag(pub String);

impl Borrow<str> for Tag {
fn borrow(&self) -> &str {
&self.0
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_tag_hash_map_str_access() {
use std::collections::HashMap;
let mut m: HashMap<Tag, i32> = HashMap::new();
m.insert(Tag("accuracy".to_string()), 1);
m.insert(Tag("loss".to_string()), 2);
// We can call `get` given only a `&str`, not an owned `Tag`.
assert_eq!(m.get("accuracy"), Some(&1));
Copy link
Contributor

Choose a reason for hiding this comment

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

Super noob question: What would &1 mean? Is it referring to reference to an i32 1? If so, is it stable and refers to the same value to L81?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would &1 mean? Is it referring to reference to an i32 1?

Yep! (where the i32 part is implied from type-checking)

If so, is it stable and refers to the same value to L81?

Nope! It’s like:

m.insert(Tag("accuracy".to_string()), 1);  // line 81

let tmp = 1;
assert_eq!(m.get("accuracy"), Some(&tmp));  // line 84
// `tmp` is implicitly dropped here

We can’t get a reference to the 1 from line 81; not only is it out of
scope, it probably never had a stack location at all (would be passed in
a register for this signature). But that’s okay, because equality on
references is defined in terms of equality on the underlying values:

In addition, the comparison operators transparently defer to the
referent's implementation, allowing references to be compared the same
as owned values.

You can get reference equality semantics by casting to pointers. This
is safe: it’s only dereferencing pointers that’s unsafe. But I’ve
never had to use this, even once. Value equality is basically always
what you want.

assert_eq!(m.get("xent"), None);
}

#[test]
fn test_wall_time() {
assert_eq!(WallTime::new(f64::INFINITY), None);
assert_eq!(WallTime::new(-f64::INFINITY), None);
assert_eq!(WallTime::new(f64::NAN), None);

assert_eq!(f64::from(WallTime::new(1234.5).unwrap()), 1234.5);
assert!(WallTime::new(1234.5) < WallTime::new(2345.625));

let mut actual = vec![
WallTime::new(123.0).unwrap(),
WallTime::new(-456.0).unwrap(),
WallTime::new(789.0).unwrap(),
];
actual.sort();
let expected = vec![
WallTime::new(-456.0).unwrap(),
WallTime::new(123.0).unwrap(),
WallTime::new(789.0).unwrap(),
];
assert_eq!(actual, expected);
}
}