Compromise: Changes to nlp clone still affects original nlp

[Drache93]

Hi I'm trying to use multiple instances of nlp each with different plugins nlp.clone().extend(). However, since the world variable is global, any changes effect all nlp instances.

This can be seen by updating the following test and adding a copy of the first check to the end.

https://github.com/spencermountain/compromise/blob/51f1f158e6fcab14cdbf14f182b0c41f94e513e7/tests/plugin/addWords.test.js#L6-L8

test('persistent-lexicon-change', function(t) {
  let nlp2 = nlp.clone()
  let doc = nlp('he is marko')
  t.equal(doc.match('#Place+').length, 0, 'default-no-place')
  t.equal(doc.match('#Person+').length, 1, 'default-one-person')

  nlp2.extend((Doc, world) => {
    world.addWords({
      marko: 'Place',
    })
  })
  doc = nlp2('he is marko')
  t.equal(doc.match('#Place+').length, 1, 'now-one-place')
  t.equal(doc.match('#Person+').length, 0, 'now-no-person')

 // ....

  // Tests fail here - original nlp should not be affected?
  doc = nlp('he is marko')
  t.equal(doc.match('#Place+').length, 0, 'default-no-place')
  t.equal(doc.match('#Person+').length, 1, 'default-one-person')

  t.end()
})

Am I misunderstanding the intention behind the clone() function? Thanks in advance!

[spencermountain]

hey Dominic, you're right. this is a little awkward right now.
This should improve in subsequent versions.
i think the solution i'd use right now would look something like this:

let oldWorld = nlp().world.clone()

nlp.extend(() => {}) //do whatever

let oldDoc = nlp.tokenize('foo bar') //don't tag it yet
oldDoc.world = oldWorld //swap in other world object
oldDoc.tagger() // tag it
oldDoc.debug()

urgh. I dunno. That looks silly.
I've got a headache this afternoon, but will return to this when i can think more clearly.

[Drache93]

Oooh, I was thinking about swapping in worlds before matching, but I wasn't sure how to access it. (fyi, try less/more coffee :D)

Update: oldDoc.world is read only!

Update 2: Ignoring that, If I want to make a few instances, I would have to get the oldWorld, nlp.extend, get the world nlp().world, revert the world back, then extend again, silly but should world. Only obvious issue is having to run nlp() before being able to set/get the world. As far as I looked, nlp() and the functions inside of it don't return early if no text is given.

[spencermountain]

ahh, i'm sorry. I'll make it writable now.
Then in the new year, i'll make this all less stupid.

[Drache93]

That's awesome, thanks! Will give it a go when it's up.

[spencermountain]

Doc.world should be writable now. Sorry for the fuss.

(12.2.1)

[Drache93]

Thanks @spencermountain ! Works great!
End result:

const originalWorld = nlp().world.clone();

function myNLP(text: string){
  // @ts-ignore
  nlp('').world = originalWorld;
  nlp.extend();

  return nlp(text);
}

function myOtherNLP(text: string){
  // @ts-ignore
  nlp('').world = originalWorld;
  nlp.extend();

  return nlp(text);
}

As long as I don't do any async stuff :D

Thanks for the help, enjoy the holidays! And Happy New Year in advance!

Edit: Second try...was trying to create a function for creating the worlds ahead of time to swap them in. However nlp().world and nlp().world.clone() don't return the same thing, clone() is basically returning a copy of the original world, not the current one.

const extendWorld = (plugin: Plugin) => {
  // @ts-ignore
  const originalWorld = nlp().world.clone();
  nlp.extend(plugin);
  const newWorld = nlp().world.clone();
  nlp().world = originalWorld;

  return newWorld;
};

Feel free to ignore this for the rest of the year.

[Drache93]

@spencermountain Tried a little experiment with this:

const tokenize = require('./01-tokenizer')
const version = require('./_version')
const World = require('./World/World')
const Doc = require('./Doc/Doc')

const globalWorld = new World()

function instance() {
  //blast-out our word-lists, just once
  let world = globalWorld.clone()

  /** parse and tag text into a compromise object  */
  const nlp = function(text = '', lexicon) {
    if (lexicon) {
      world.addWords(lexicon)
    }
    let list = tokenize.fromText(text, world)
    let doc = new Doc(list, null, world)
    doc.tagger()
    return doc
  }

  /** parse text into a compromise object, without running POS-tagging */
  nlp.tokenize = function(text = '', lexicon) {
    if (lexicon) {
      world.addWords(lexicon)
    }
    let list = tokenize.fromText(text, world)
    let doc = new Doc(list, null, world)
    return doc
  }

  /** mix in a compromise-plugin */
  nlp.extend = function(fn) {
    fn(Doc, world)
    return this
  }

  /** make a deep-copy of the library state */
  nlp.clone = function() {
    world = world.clone()
    return this
  }

  /** re-generate a Doc object from .json() results */
  nlp.load = function(json) {
    let list = tokenize.fromJSON(json, world)
    return new Doc(list, null, world)
  }

  /** log our decision-making for debugging */
  nlp.verbose = function(bool = true) {
    world.verbose(bool)
    return this
  }

  /** create instance using global world */
  nlp.instance = function() {
    return instance()
  }

  /** current version of the library */
  nlp.version = version
  // alias
  nlp.import = nlp.load

  return nlp
}

module.exports = instance()

Instance wrapper for nlp - probably massively breaking...
Basically all new instances use a clone of the "global world" meaning you can still share plugins etc. but also make local changes separately

[spencermountain]

I LIKE THAT!!

[spencermountain]

apologies, maybe the coffee hasn't kicked-in yet.

is it possible to get one nlp where marko is a place, and one where it's a person now? I haven't been able to figure it out yet.

let nlp1 = nlp.instance()
let nlp2 = nlp.instance()
nlp2.extend((Doc, world) => {
  world.addWords({ marko: 'Place' })
})
nlp('marko').debug() // Place
nlp1('marko').debug() // Place
nlp2('marko').debug() // Place

That's a huge improvement, if that's possible.
cheers

[Drache93]

That's the idea! Worked in the tests:

  const instance = nlp.instance().extend(plugin)
  const instance2 = nlp.instance()

  t.equal(instance().world.test, 'test')
  t.equal(instance2().world.test, undefined)

[spencermountain]

ah, thank you.
yeah, this is looking great.

I guess we don't really need nlp.clone anymore - it doesn't (and never did) produce an independent copy. I can remove it in the next major release.

any feelings on alternate names instead of .instance()?
Do you have any feelings about .original()?? .default()? ... .defacto()?
Been paging through my thesaurus and that's the best I've come up with.

[Drache93]

Haha :D .new feels weird in JS. Also it's not original or default considering it's still uses the global world (could always add a Boolean for totally fresh)

Shame clone is used :D copy()?

Edit: Maybe the global world thing is confusing. Maybe with argument or extra function the choices are: new nlp with world cloned from old one, or nlp with fresh world. No global extend/world.

[Drache93]

Could export the instance function as a contractor and use it as new NLP()? Think that's do able...

[spencermountain]

right, hmm...

You're free to change .clone() in any way - that would be best, if it deep-copied whatever dirty state the library is at. I'm not sure if that's an easy change or not.

I'm thinking too about memory-usage - if we had to keep a fresh copy of World around, that may double our memory usage.
Although that doesn't seem to be a problem anywhere, these days.
up to you.

[Drache93]

True, but we'd only do it because a person specifically chose to make a fresh instance, so their choosing to using more memory :D.

Maybe we just fix clone to create a fresh instance with cloned world? Breaking change if people are using it currently with the assumption changes go backwards. So no global world, and no new worlds. Just selective cloning. Again can always have a Boolean for empty clone.

[spencermountain]

👌