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

Missing tests #18

Closed
pb-cdunn opened this issue May 15, 2018 · 18 comments
Closed

Missing tests #18

pb-cdunn opened this issue May 15, 2018 · 18 comments

Comments

@pb-cdunn
Copy link
Contributor

I see no tests for pack_type/unpack_type.

I need to see those so I can understand what I'm doing wrong. I have this:

proc unpack_type*(ss: MsgStream, x: var str9) =
  var str: string
  ss.unpack(str)
  copyMem(addr x[0], addr str[0], 9)

And I get this:

falcon/rr_hctg_track.nim(65, 5) Error: type mismatch: got <MsgStream, string>
but expected one of:
proc unpack[T](data: string; val: var T)
  first type mismatch at position: 1
  required type: string
  but expression 'ss' is of type: MsgStream

It worked with older msgpack and nim-0.18.0. I have tried this:

-proc unpack_type*(ss: streams.Stream, x: var str9) =
+proc unpack_type*(ss: MsgStream, x: var str9) =
   var str: string
-  msgpack4nim.unpack(ss, str)
+  ss.unpack(str)
   copyMem(addr x[0], addr str[0], 9)
@pb-cdunn
Copy link
Contributor Author

Missing in examples/ too. I see this:

 23 s.pack(x) #here the magic happened
 24
 25 s.setPosition(0)
 26 var xx: CustomType
 27 s.unpack(xx) #and here too

I will try that. But I don't see how to provide (un)pack_type() for any new type.

@pb-cdunn
Copy link
Contributor Author

403   let rtn = tr_stage1(infile, fn, min_len, bestn)
...
412   var ss = streams.newFileStream(fn_rtn, system.fmWrite)
413   defer: ss.close()
415   msgpack4nim.pack(ss, rtn)

I am unable to get it working with the new magic (and I'm not convinced the magic would work for me anyway, since I have some raw data).

falcon/rr_hctg_track.nim(415, 19) Error: type mismatch: got <FileStream, myprioritytable>
but expected one of:
proc pack[T](s: var MsgStream; val: T)
  first type mismatch at position: 1
  required type: var MsgStream
  but expression 'ss' is of type: FileStream
proc pack[T](val: T): string
  first type mismatch at position: 1
  required type: T
  but expression 'ss' is of type: FileStream

@pb-cdunn
Copy link
Contributor Author

82de886 works fine for me, without any changes to my code. Please help me to upgrade my code.

Otherwise, I may have to lock on that older version.

cdunn2001 pushed a commit to bio-nim/nim-falcon that referenced this issue May 15, 2018
@jangko
Copy link
Owner

jangko commented May 16, 2018

please try this, looks like you're missing the var modifier. The decision to change the Stream to MsgStream is for performance reason. Sorry if the documentation is not updated properly.

proc unpack_type*(ss: var MsgStream, x: var str9) =
  var str: string
  ss.unpack(str)
  copyMem(addr x[0], addr str[0], 9)

for your second example:

403   let rtn = tr_stage1(infile, fn, min_len, bestn)
...
412   var ss = streams.newFileStream(fn_rtn, system.fmWrite)
413   defer: ss.close()
      var msgss = initMsgStream() #you can pass some buffer capacity here
415   msgpack4nim.pack(msgss, rtn)
      # later when you need to write to file
      ss.write(msgss.data)

I know this pattern is less convenience than previous version, but the runtime performance is better.

@jangko
Copy link
Owner

jangko commented May 16, 2018

my long term goal is to use Nim's "concepts" so this library can use whatever stream compatible object, but for the moment, Nim's "concepts" is plague with bugs.
while waiting for more stable "concepts", I prefer performance than convenience, because msgpack4nim is also used for rpc intensive application

@pb-cdunn
Copy link
Contributor Author

pb-cdunn commented May 20, 2018

Yeah, I too lament the problems with concepts.

@pb-cdunn
Copy link
Contributor Author

falcon/rr_hctg_track.nim(413, 19) template/generic instantiation from here
msgpack4nim/msgpack4nim.nim(667, 30) Error: undeclared field: 'data'
        s.pack_type undistinct(field)
                               ^
412   var msgss = initMsgStream()
413   msgpack4nim.pack(msgss, rtn)

That didn't quite work. Ideas?

@pb-cdunn
Copy link
Contributor Author

rtn is of type

tables.initTable[string, binaryheap.Heap[mytuple]]

https://github.com/bio-nim/nim-heap

Kind of complex. But this used to work fine.

cdunn2001 pushed a commit to bio-nim/nim-falcon that referenced this issue May 20, 2018
@jangko
Copy link
Owner

jangko commented May 21, 2018

add import msgpack4collection
Nim standard library collections pack-unpack are moved to msgpack4collection, they are no longer part of codec core

@pb-cdunn
Copy link
Contributor Author

I have compiled pack() on a MsgStream from a Nim stream now. But what's the equivalent for unpack()?

Your front-page docs really need to show this. Nobody wants to learn the details of MsgStream. I don't mind the intermediate data-structure; I'm all for efficiency. I just need an example.

@pb-cdunn
Copy link
Contributor Author

let infile = streams.newFileStream(fn_rtn, system.fmRead)
defer:
    infile.close()
var msgss = msgpack4nim.initMsgStream()
msgss.data = infile.readAll()

That works. (I'd rather you provide something to interact with Nim standard Streams, so I don't have to access internals of MsgStream.)

But now I see a problem. One of the reasons to use msgpack format is that it very easily allows low-overhead reading of large datafiles. Each msgpack "thing" tells you in advance how much bytes to read.

But you are forcing me to read the entire string into memory! That's a significant problem.

I'm fine with having a mode that's particularly time-efficient, but I also need a mode that is memory-efficient.

I guess it's not a huge problem, since it only doubles memory at most (since the data will exist in memory after parsing anyway). But could you discuss the trade-off and why you made it?

cdunn2001 pushed a commit to bio-nim/nim-falcon that referenced this issue May 22, 2018
* jangko/msgpack4nim#18

And update both msgpack4nim and cligen.
cdunn2001 pushed a commit to bio-nim/nim-falcon that referenced this issue May 22, 2018
* jangko/msgpack4nim#18

And update both msgpack4nim and cligen.
@jangko
Copy link
Owner

jangko commented May 22, 2018

i am not aware of your situation, I thought of common msgpack usage when I made those changes.
usually msgpack usage is memory to memory, or process to process communication, and seldom involve physical file.
But after I see your use case, I think I must revisit my long term goal to provide a generic interface that can be tuned to various use case and finer control of decoding step.

@pb-cdunn
Copy link
Contributor Author

usually msgpack usage is memory to memory

I worked with msgpack at Amazon on the service security layer. (If you've shopped at Amazon, you have used msgpack on a stream.) Believe me; this is a common use-case.

Anyway, it's only a factor of 2x, not a big deal, memory-wise.

However, an example of how to convert between MsgStream and Nim streams is a big deal. We shouldn't be going into the internals of MsgStream, I think.

@jangko
Copy link
Owner

jangko commented May 22, 2018

right, I assume you want something like this

var s = initMsgStream(someStream)

@pb-cdunn
Copy link
Contributor Author

Hmm. That's probably good enough, if you want to add state to MsgStream. (We need both directions.)

Or without changing MsgStream, simply provide a function which takes both a MsgStream and and Nim stream and writes to the stream, and another function which reads from a stream. That's just to hide the implementation details of MsgStream.

@jangko jangko closed this as completed in eb13855 May 23, 2018
@pb-cdunn
Copy link
Contributor Author

pb-cdunn commented Jul 2, 2018

Ugh. I thought I had this working.

falcon/rr_hctg_track.nim(414, 19) template/generic instantiation from here
/localdisk/scratch/cdunn/repo/fork/repos/nim-falcon/repos/msgpack4nim/msgpack4nim.nim(1031, 53) template/generic instantiation from here
/localdisk/scratch/cdunn/repo/fork/repos/nim-falcon/repos/msgpack4nim/msgpack4collection.nim(38, 4) template/generic instantiation from here
/localdisk/scratch/cdunn/repo/fork/repos/nim-falcon/repos/msgpack4nim/msgpack4nim.nim(657, 6) template/generic instantiation from here
/localdisk/scratch/cdunn/repo/fork/repos/nim-falcon/repos/msgpack4nim/msgpack4nim.nim(699, 32) Error: undeclared field: 'data'
          s.pack_type undistinct(field)
                                 ^

What could cause that? (I'm using nim/0.18.1)

@pb-cdunn
Copy link
Contributor Author

pb-cdunn commented Jul 2, 2018

This version of msgpack4nim worked: c95e495

So I guess I need to alter my code again? This is what I had:

@jangko
Copy link
Owner

jangko commented Jul 2, 2018

sorry for all the trouble, msgpack4nim now can works with stringstream, filestream, and msgstream.
basically you can use your previous code again, but you still need to import msgpack4collection.
while the current situation looks like you need to remove the 'var' modifier from 'var MsgStream' line 64-73, because MsgStream now is a ref object.
it is not clear why nim cannot call pack_type with 'var MsgStream' for ref object.

EDIT: you can also use generic version of pack_type/unpack_type, you can see the example in readme.md#example

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

No branches or pull requests

2 participants