Reference time support #78

niedakh · 2018-07-17T11:41:12Z

This PR adds an additional parameter to provide a reference timestamp for datetimes detected by rustling.

…develop

niedakh · 2018-07-17T13:29:12Z

note that my Cargo.ml repository url changes should be reverted before merging

adrienball

Thanks a lot for this PR, I made some comments.

adrienball · 2018-07-17T14:43:18Z

platforms/snips-nlu-ontology-python/snips-nlu-ontology-rs/Cargo.toml

-snips-nlu-ontology-parsers-ffi-macros = { git = "https://github.com/snipsco/snips-nlu-ontology", branch = "develop" }
+snips-nlu-ontology = { git = "https://github.com/niedakh/snips-nlu-ontology", branch = "develop" }
+snips-nlu-ontology-ffi-macros = { git = "https://github.com/niedakh/snips-nlu-ontology", branch = "develop" }
+snips-nlu-ontology-parsers-ffi-macros = { git = "https://github.com/niedakh/snips-nlu-ontology", branch = "develop" }


There is a script (.travis/install.sh) which is run during the CI to replace these dependencies with relative paths.

That means you can discard these changes :)

adrienball · 2018-07-17T15:04:03Z

snips-nlu-ontology-parsers/src/builtin_entity_parser.rs

@@ -25,17 +28,20 @@ lazy_static! {
 }

 impl BuiltinEntityParser {
-    pub fn new(lang: Language) -> Self {
+    pub fn new(lang: Language, reference_timestamp: i64) -> Self {


I think that the reference_timestamp should be provided as a parameter of the extract_entities method, in order to avoid the creation of multiple BuiltinEntityParser.

Besides, this parameter should be optional.

good idea, I'll move this to extract_entities in a moment. I'm wondering on which tier it needs to be optional. It already is optional in the python binding, I can make it optional using Option in rust, but it will have to be "non-optional" in the ffi. Does that sound ok?

adrienball · 2018-07-17T15:05:42Z

snips-nlu-ontology-parsers/src/builtin_entity_parser.rs

@@ -107,7 +113,11 @@ impl BuiltinEntityParser {
        sentence: &str,
        filter_entity_kinds: Option<&[BuiltinEntityKind]>,
    ) -> Vec<BuiltinEntity> {
-        let context = ResolverContext::default();
+        // don't cover global nlu_ontology::Grain
+        use rustling_ontology::{Grain, Interval};


I'm not sure I understand why you do the import here.

adrienball · 2018-07-18T08:39:13Z

snips-nlu-ontology-parsers-ffi-macros/src/builtin_entity_parser.rs

@@ -101,7 +104,7 @@ pub fn extract_entity(
    };
    let opt_filters = opt_filters.as_ref().map(|vec| vec.as_slice());

-    Ok(parser.extract_entities(sentence, opt_filters))
+    Ok(parser.extract_entities(sentence, Some(reference_timestamp), opt_filters))
 }


I think you should make reference_timestamp optional here through a -1 value, not ideal but would let the rust code handle the default logic. The ffi may be used by other bindings than just the python one.

Done, I went with a i64:MIN for default value, instead of -1, it is much less probable that someone wants to set reference time to 1901-12-13 20:45:52 than 1969-12-31 23:59:59.

adrienball · 2018-07-18T08:59:18Z

snips-nlu-ontology-parsers/src/builtin_entity_parser.rs

+        );
+        let context = ResolverContext::new(
+            Interval::starting_at(Moment(reference_datetime), rustling_ontology::Grain::Second)
+        );


I would rather go for the following, in order to reduce the number of created objects and let rustling define the default behavior:

let context = reference_timestamp .map(|ts| ResolverContext::new( Interval::starting_at(Moment(Local.timestamp(ts, 0)), rustling_ontology::Grain::Second))) .unwrap_or_else(|| ResolverContext::default());

niedakh · 2018-07-25T11:26:22Z

@adrienball i've fixed the code, added a default value case (i64::min), all test pass, do you have a moment for another iteration of the review?

adrienball · 2018-07-25T14:50:10Z

Hey @niedakh ,
After re-reading your PR, I'm thinking that, instead of updating the current API we should add a new one which would be something like:

pub fn extract_entity_with_context(
     ptr: *const CBuiltinEntityParser,
     sentence: *const libc::c_char,
     filter_entity_kinds: *const CStringArray,
     reference_timestamp: i64,
 ) -> Result<Vec<BuiltinEntity>>

This has two advantages:

it eliminates the reference_timestamp == i64::MIN condition which doesn't look good IMO
it avoids breaking changes, as it will be backward compatible

With that in mind, the python wrapper should call either the existing API or the new one, depending on wether or not a reference_timestamp was provided.

What do you think?

niedakh · 2018-08-06T12:01:41Z

@adrienball I agree, this makes a lot of sense, I should be able to update the PR this week

adrienball · 2018-08-08T12:46:23Z

BTW, we bumped the version of rustling-ontology in this PR, and now Moment, Local and TimeZone types are re-exported by rustling-ontology so you should be able to remove the chrono and rustling-ontology-moment dependencies.

Also, the ResolverContext struct now has a new builder:

struct ResolverContext {
    pub fn from_nsecs(nsecs: i64) -> ResolverContext;
}

which you can use directly.

niedakh added 6 commits July 16, 2018 16:58

reference time support

23023a9

enforce timezone in reference time

4f8f0d1

unit tests added

f403a39

switch to local time and timestamp instead of isostrings

bab1888

remove offset from test

801df20

Merge branch 'develop' into develop

b089a21

niedakh mentioned this pull request Jul 17, 2018

Ability to pass reference date context #74

Open

niedakh added 2 commits July 17, 2018 13:46

python2 compatible timestamp fallback

cd7d32a

Merge branch 'develop' of github.com:niedakh/snips-nlu-ontology into …

34863aa

…develop

adrienball requested changes Jul 17, 2018

View reviewed changes

niedakh added 6 commits July 18, 2018 07:28

move reference time to parse(), make optional

6670d83

fixes

b1c23ee

compile fix

7fa5535

more compile fixes

8ca1dd5

another compile fix

f61ece2

fix passing ref_time in tests

6863af5

adrienball reviewed Jul 18, 2018

View reviewed changes

niedakh added 7 commits July 19, 2018 09:07

better unwrap by adrian

79466da

default value set

7d51ca9

revert repo changes in cargo.toml

ef51d2d

fix compile problem

e2cd335

more compile fixes

a0e9fa1

compile fixes

4e55bef

variable rename

6a2a781

adrienball closed this Jul 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reference time support #78

Reference time support #78

niedakh commented Jul 17, 2018

niedakh commented Jul 17, 2018

adrienball left a comment

adrienball Jul 17, 2018

adrienball Jul 17, 2018

adrienball Jul 17, 2018

niedakh Jul 18, 2018

adrienball Jul 17, 2018

adrienball Jul 18, 2018

niedakh Jul 23, 2018

adrienball Jul 18, 2018 •

edited

Loading

niedakh Jul 23, 2018

niedakh commented Jul 25, 2018

adrienball commented Jul 25, 2018

niedakh commented Aug 6, 2018

adrienball commented Aug 8, 2018

Reference time support #78

Reference time support #78

Conversation

niedakh commented Jul 17, 2018

niedakh commented Jul 17, 2018

adrienball left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrienball Jul 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niedakh commented Jul 25, 2018

adrienball commented Jul 25, 2018

niedakh commented Aug 6, 2018

adrienball commented Aug 8, 2018

adrienball Jul 18, 2018 •

edited

Loading