Closed PrivacyMatter closed 4 years ago
Hi @PrivacyMatter. Thanks for the report. I may very well have missed that change. As I mentioned in the commit message, I was not able to easily identify the functional changes because of all the formatting changes.
I just pushed the two branches I used to compare (1) the version of master Soren's PR was based on, with (2) the changes in his PR. I would really appreciate it if you could take a look at them and see if you can verify that I indeed missed something he changed.
Hi, Ok, I took a look, and something definitely happened there. I'm far from being a git expert, so it's not clear to me exactly how and what happened, but I'll describe what I'm seeing:
The one clue that I noticed is that when I compare twsIDs on the branch used as the master for Soren's PR to the first version on the branch you used for applying the changes, the file appears in a "renamed" state. Maybe something happened there that I'm missing, but the bottom line is that the changes you made on 113ec06 disappeared together with the file by a following commit on the same branch.
A possibly related issue: after pulling the latest version on master, I encountered another problem: I could no longer connect to the server (I'm using the latest IB Gateway version). Some investigation of the logs in gateway indicated that it is probably a mismatch in the memory representation of some structures. Changing CLIENT_VERSION <- "63"
in twsConnect function (twsConnect.R) to the older "47" value solves the problem.
I noticed now that some of the missing changes in twsIDs included the following lines:
`.IBAPI.VERSION` <- 'clientV63' #client Version 63
I was hoping that introducing this line by applying the changes in twsIDs.R will solve the problem, but it doesn't look like it does. Even after I copied the version of twsIDs from commit 113ec06 and rebuilt the library, I cannot connect to the server unless I change the CLIENT_VERSION
value in twsConnect to be 47.
I will appreciate your input on that.
Thanks, Yossi
Hi again,
Ok, so it seems that there are many additional problems with the message structure and the connection process, as many changes that were done on the lint-censix branch and were not migrated to the trunk, so using twsIDs.R for version 63 does not work either.
In particular, twsConnect at the head of lint-censix has many changes to twsConnect when comparing it to the branch root --- only one of those is the update of CLIENT_VERSION
that was migrated to the trunk (the master branch). It seems that twsConnect was significantly changed on the the lint-censix branch --- e.g. an internal startApi function was introduced, plus many other changes. The only change that was migrated to the trunk is the update to CLIENT_VERSION
, though, which may explain why reverting this change back to 47 together with using the old twsIDs.R file worked, even though the rest of the code was modified to support version 63.
At the bottom line, the problem seems to be somewhat more complex than I thought:
CLIENT_VERSION
to 47 doesn't work either with the updated version of twsIDs.R because it matches the structures memory footprint of version 63, not 47.The easiest way to get the library to work for me at the moment is to revert to twsIDs.R of version 47, and change twsConnect to identify itself as client version 47. That does not match the rest of the code on the trunk that assumes version 63, but so far the functionality that I need happens to work despite this mismatch.
I may try to investigate further but I don't have much time for it so will probably use the workaround of using the trunk version and modify CLIENT_VERSION
to be 47. Will appreciate if you can comment on that as you obviously know the code better than I do.
Thanks for your detailed analysis! I'll review this weekend.
Thanks again for taking the time to investigate these issues!
The one clue that I noticed is that when I compare twsIDs on the branch used as the master for Soren's PR to the first version on the branch you used for applying the changes, the file appears in a "renamed" state.
Yeah, that is a consequence of how I tried to make the two branches easier to compare. I used package.skeleton()
to create the source files. It creates one file for each function. The source code wasn't organized that way, so the contents of the files didn't align with the master branch.
I haven't had time to try and replicate your analysis, but I did do some work to try and make the differences more apparent. I just pushed 3 new branches that call styler::style_dir("R/")
on:
I tried to remove as many of the whitespace changes from Soren's "styled" PR branch. That should make it easier to compare the 3. I haven't tried to compare them yet, and I likely won't have time to do it for a week or so. I'd really appreciate any help!
Hi, will try to get to it. As I mentioned I think there was a problem at least with twsConnect as things didn’t work right with either versions assigned to, but will try to see if that was solved when I have the time to test/look at it. Not sure exactly when, in the midst of some other things. BTW I happened to meet a guy named William Kats from IB last week in a webinar, and apparently they assigned someone new to work on the API/Gateway front, and asked me for some comments as he meets the guy this week. Didn’t get to this either, but he also said that they have many clients that use R but they all use this library which is how they make them happy. For now . My hope is to try pushing them to support R interface directly. Hopefully your include doesn’t depend on maintaining IBrokers 😄. (I doubt that they will do and definitely not soon, but will try to see what’s possible). Thanks for doing this! Yossi
I looked at this and... wow, I missed a lot. Please don't spend time investigating until this gets cleaned up. I started working on it yesterday, and will continue to work on it this week.
Great, thanks, just saved me some tine I don’t have :) Drop me a message when things look more stable And I’ll take a look then. BTW: there is a tiny bug that was there way before you touched it, so don’t have it in front of me but will look later on and drop a message. Something about using either con[[1]] or conn[[1]], or something similar towards the end of reqMktData. Will check later.
Thanks, Yossi
Sent from my iPhone
On Sep 16, 2019, at 7:24 AM, Joshua Ulrich notifications@github.com wrote:
I looked at this and... wow, I missed a lot. Please don't spend time investigating until this gets cleaned up. I started working on it yesterday, and will continue to work on it this week.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.
Hi, so i just saw it via github. In
https://github.com/joshuaulrich/IBrokers/blob/d098292ba35782c3e5adcf5e533a9ddb47c1cf8e/R/reqMktData.R#L206
There is using con[[1]]
is incorrect, I think. con
is defined as conn[[1]]
(I.e the actual connection vs the twsConnection
object) — see https://github.com/joshuaulrich/IBrokers/blob/d098292ba35782c3e5adcf5e533a9ddb47c1cf8e/R/reqMktData.R#L83 so it should be either con
or conn[[1]]
, I think.
The two commits I just pushed attempt to restore all the functionality I missed. I'm able to connect to TWS now, and I'm able to pull currency data using the TWS demo.
conn <- twsConnect()
currency <- twsCurrency("EUR")
reqMktData(conn, currency)
TWS Message: 2 1 300 Can't find EId with tickerId:1
TWS Message: 2 -1 2108 Market data farm connection is inactive but should be available upon demand.usfuture
TWS Message: 2 -1 2108 Market data farm connection is inactive but should be available upon demand.usfuture
<20190919 07:04:54.339495> id=1 symbol=EUR bidPrice: 1.10704 bidSize: 5000000
<20190919 07:04:54.340327> id=1 symbol=EUR askPrice: 1.10705 askSize: 2995000
<20190919 07:04:54.340815> id=1 symbol=EUR bidSize: 5000000
<20190919 07:04:54.341098> id=1 symbol=EUR askSize: 2995000
<20190919 07:04:54.341475> id=1 symbol=EUR lastSize: 0
<20190919 07:04:54.341817> id=1 symbol=EUR highPrice: 1.10740
<20190919 07:04:54.342205> id=1 symbol=EUR lowPrice: 1.10230
<20190919 07:04:54.342559> id=1 symbol=EUR closePrice: 1.10300
<20190919 07:04:54.343188> id=1 symbol=EUR Halted: 0.0
That's all I was able to test this morning. I'd appreciate any testing you can do. Thanks!
Thanks Joshua. I am in the midst of extending my use of the library for quite a bit, adding code that also put orders, etc (yes, testing on paper account first); the plus side is that I'll hopefully be able to do quite a deeper / more thorough testing, but OTOH it will probably take a few days till I get to it.
One thing that I noticed is that the bug I mentioned in my previous message is still there (using con[[1]]
instead of con
or conn[[1]]
): https://github.com/joshuaulrich/IBrokers/blob/19695d6ad7c39a383126eaff47217f726ff2b993/R/reqMktData.R#L206
This bug does fire for me, not sure why it doesn't on your tests -- am I missing something?
I didn't test that line yesterday. I tested it now, and it does not throw an error for me. I agree that it looks like con[[1]]
should be con
or conn[[1]]
. That said, I cannot replicate an issue. Can you give me a small example that causes the behavior, and the output/error you see?
In my tests, con[[1]]
returns the number of the connection in the output of showConnections(TRUE)
. And isOpen()
seems to be happy with an integer value for the connection, instead of the connection object itself.
EDIT: My comment isn't meant to suggest that I won't make your suggested change. I probably will. But I would like to understand the problem.
More importantly, though, I think that I was wrong saying that this bug is the cause for the error that I got, as I've done some other changes due to other issues I had to work around, and I think I commented out some code that probably is the reason for the error that I got;
- if (!is.twsConnection(conn))
- stop("tws connection object required")
-
- if(!is.twsPlayback(conn)) {
- Contract <- as.twsContract(Contract)
- if(is.twsContract(Contract)) Contract <- list(Contract)
-
- for(n in 1:length(Contract)) {
- if (!is.twsContract(Contract[[n]]))
- stop("twsContract required")
- }
-
- } #else file <- ""
+ # if (!is.twsConnection(conn))
+ # stop("tws connection object required")
+ #
+ # if(!is.twsPlayback(conn)) {
+ # Contract <- as.twsContract(Contract)
+ # if(is.twsContract(Contract)) Contract <- list(Contract)
+ #
+ # for(n in 1:length(Contract)) {
+ # if (!is.twsContract(Contract[[n]]))
+ # stop("twsContract required")
+ # }
+ #
+ # } #else file <- ""
when I dealt with the issue with playback that you (and I, locally) later fixed. Then I applied only the fix of the con[[1]] vs con
bug on a fresh copy of the function, and it made it seem as if the problem is due to that bug (I basically had two versions of the library, one with only that fix and one with the other buggy workaround but without that fix, which was the source of my confusion).
Really sorry for the confusion I caused. While it is indeed a bug that might manifest at some point (there are no guarantees that the structure of R connection object or the implementation of isOpen will remain the same), it's definitely not as important to address it as I thought. It's probably still a good idea to fix it regardless before the next push to the trunk, and hopefully to CRAN.
As for testing: writing the code that uses the library more heavily takes a bit longer than expected (as these things always do); I do hope to have some code that uses the library to monitor and make actual trades with a paper account at some point during next week. That's my highest propriety at the moment in the project I'm working on. Once I have that, it will be a good opportunity to test the latest version on the branch.
- Really sorry for the confusion I caused.
No worries! And thanks for the detailed explanation. I'm going to make your suggested change and close this issue. Please open new issues for new problems you find (one issue per problem).
Thanks again for all your contributions! You have been incredibly helpful!
Hi,
I noticed that commit a9ae056 introduced support for new incoming message types, such as MARKET_DATA_TYPE, COMMISSION_REPORT, etc (codes 58:68) and they are handled in processMsg.R, e.g:
but I do not see these new message codes in twsIDs.R:
I also checked the value of .twsIncomingMSG after installing the latest version and these message indeed seems to be missing, even though the processMsg function does reference them (probably R lazy evaluation is what still allows the code to be built). Am I missing something, or that these codes were indeed left out?
Thanks.