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

backport open-before-change from #278 #284

Merged
merged 29 commits into from
Jul 2, 2020

Conversation

bollwyvl
Copy link
Collaborator

@bollwyvl bollwyvl commented Jun 19, 2020

References

Code changes

  • ws-connection maintains a map of the URIs for which it currently thinks it has sent a didOpen
  • ws-connection (and connection, too, because it has _sendChange consult this map before sending an didChange, and if it hasn't been sent, sends didOpen (which also immediately sends a didChange)
  • if anything happens that might indicate the server changing (e.g. initialize, close) the map gets cleared
  • probably needs a test, but that might just be looking at lab.log after an acceptance run to verify that that traceback doesn't occur

User-facing changes

  • pyls won't complain about files not being open in the log, which looks scary and confusing

Backwards-incompatible changes

  • N/A

Chores

  • linted
  • tested
  • documented
  • changelog entry

@bollwyvl bollwyvl changed the title [wip] backport open-before-change from #278 backport open-before-change from #278 Jun 20, 2020
Validate Lab Log
[Documentation] Ensure the notebook server log doesn't contain certain errors
${log} = Get File ${LAB LOG}
Should Not Contain Any ${log}
Copy link
Member

Choose a reason for hiding this comment

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

Optional enchantment: should we also have some positive sanity check Should Contain "lsp - extension loaded" (I think we do not have such a message but could add it) to test if we are recording the logs ok?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i'm just looking for stuff we know and can point to... not looking to add any new stuff just to look for it. If the extension doesn't load, nothing is going to work.

Also, i've changed it to fire after every test, since we want to know which test generated the offending log message(s).

@bollwyvl bollwyvl closed this Jun 20, 2020
@bollwyvl bollwyvl reopened this Jun 20, 2020
@bollwyvl
Copy link
Collaborator Author

🎉 checking the log did find an error on win36.

👻 win36 is timing out. Will do some rejiggering to see if i can figure it out. I guess we need to only check the log delta for errors.

@krassowski
Copy link
Member

krassowski commented Jun 21, 2020

Was just thinking recently about trying out GitHub actions. Do you have any experience with it? Could it help with the timeouts (and with everything else)?

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Jun 21, 2020 via email

@bollwyvl
Copy link
Collaborator Author

It appears jedi might be detecting some other python on win36, when it calls:
https://github.com/davidhalter/jedi/blob/f9183bbf6436fc4c4a384c7c8dcb45324c8720f6/jedi/api/environment.py#L183

This is generating some deep pickle errors, which cascade into a bunch of other things. Still trying to get to the bottom of it.

- r-irkernel
- r-languageserver
- r-stringi >=1.4.6
- r-xfun !=0.15
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

looking into these on conda-forge...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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


def warm_up_one(module):
start = time.time()
script = jedi.Script(SOURCE_TEMPLATE.format(module=module))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've determined the jedi caching mechanism is very broken on python 3.6...

Traceback (most recent call last):
  File "C:\Miniconda\lib\site-packages\jedi\inference\compiled\subprocess\__init__.py", line 261, in _send
    is_exception, traceback, result = pickle_load(self._get_process().stdout)
  File "C:\Miniconda\lib\site-packages\jedi\_compatibility.py", line 396, in pickle_load
    return pickle.load(file, encoding='bytes')
EOFError: Ran out of input

Have some more debugging to do...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

pre-warming the cache is apparently improving the overall windows pass rate on the first run for non-3.6 tests, which is good...

@bollwyvl
Copy link
Collaborator Author

So probably going to take a breather on this until i figure out how to unbreak jedi on win/36 at some level of upstream... very curious.

@bollwyvl
Copy link
Collaborator Author

Alright, looks like some of the issues we uncovered here are blocking other things. I'm going to mark some win/36 tests non-critical for the time being.

@bollwyvl bollwyvl mentioned this pull request Jun 30, 2020
22 tasks
@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Jul 1, 2020

Well, it's green: I'm not happy with having to skip so much on win/py36, but it's blocking other stuff.

@bollwyvl bollwyvl requested a review from krassowski July 1, 2020 00:05
Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

I'm fine with the code changes, let's just document the Jedi workarounds better, please :)

scripts/jedi_cache.py Show resolved Hide resolved
@krassowski krassowski merged commit f173fbc into jupyter-lsp:master Jul 2, 2020
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

Successfully merging this pull request may close these issues.

2 participants