Investigate pdb setups

pdb has been disabled - we should file an issue to investigate
if we want it - jupyterhub/zero-to-jupyterhub-k8s#1938

I'm strongly opinionated after having researched and considered this in depth.
I find there is no point enabling a PDB to enforce either 1 replica availability or 0 replica unavailability on pods not supporting having two separate replicas on - this was the old z2jh default.
Note also that mybinder.org-deploy has explicitly opted out of the old z2jh default, and systematically avoids having behavior like the old z2jh default for all other deployments with 1 replica. This was discussed in https://github.com/jupyterhub/mybinder.org-deploy/pull/1730#issuecomment-751519417 also.
Originally posted by @consideRatio in https://github.com/2i2c-org/pilot-hubs/issues/194#issuecomment-774046040

So the behavior I want is that no node that contains a hub or proxy pod should ever be automatically deleted (from autoscaling, automatic upgrades, etc) without manual intervention. @consideRatio do you know how we can accomplish that?

Assuming all automatic deletions will respect PDBs which only work by denying eviction requests, then you do it in the way I was strongly opinionated against ;)
But, assuming you want specifically to stop automatic deletion with regards to cluster downscaling (but not manual k8s node upgrades, or automated maintenance windows doing that), you preferably block cluster downscaling by adding an annotation to make the cluster downscaler don't try to drain a node with such pod on it in the first place.
Btw, I'm not confident a maintenance window upgrade will repspect the PDBs indefinitively, perhaps they only try to for 10 minutes or so and then go with it for example.

so I think the hub won't be able to run with multiple replicas for a long, long time :) Until then, I wanna make sure that there's no unannounced downtime for hubs, as much as possible. I do that with the following:

Run a regional cluster, so master upgrades don't disrupt hubs
Run a core node pool
Turn off autoupgrade for the core node pool, so all upgrades are done manually
Enable PDBs, so the core nodes don't get autoscaled down if they have hub or proxy pods. I'm ok autoscaling them for other pods that might exist on them (prometheus, grafana, etc). This seems to work across cloud providers without any configuration.

What do you think of this goal, and the current steps for accomplishing it?

@yuvipanda I just want to say that I really appreciate your part of the dialogue about this with me. You just keep finding very constructive paths onwards even though it can be tricky when someone is strongly opinionated about something. I really appreciate that!
I suggest replacing point four about using PDBs with the strategy of annotating the hub/proxy pod to make the cluster autoscaler not scale them down. By doing that, we can still manually ask the cloud to do a manual node pool upgrade without also needing to do manually delete the hub/proxy pods when needed whenever that upgrade manually invoked node pool upgrade get stuck.
hub:
  annotations:
    "cluster-autoscaler.kubernetes.io/safe-to-evict": "false"

proxy:
  annotations:
    "cluster-autoscaler.kubernetes.io/safe-to-evict": "false"

Glad to work through this, @consideRatio :) Long term solution is to make things HA, of course...
Is that annotation supported by upstream autoscasler?

@yuvipanda woops updated my reply after you responded with some feedback on the goal itself and points 1-3. Also included a reference about the annotation.
I'm not 100% on what all cloud providers use to autoscale nodes, but I think they typically use that cluster autoscaler, perhaps with their own fork with smaller changes to it.

I've now come to the conclusion that @consideRatio is right, and we shouldn't have any pdbs for hub / proxy. We have a separate core node pool that won't scale down to 0, and so the pdb isn't needed for that. Its presence causes issues in node upgrades, replacements, etc - so not worth keeping.
Currently, I see we have pdbs for user placeholders & user schedulers. Do you think we should keep those, @consideRatio?
Thank you for patiently working with me on this ;0

The PDB for user-placeholder pods is to give permission for the pods to be disturbed without restrictions, it is a good default to keep.
The PDB for the user-scheduler which is HA can make sense to have as long as we have the default of 2 replicas set on the user-scheduler deployment, as it ensure there is always one running.
I want to make the following change to the z2jh user-scheduler PDB though, so it will be fine to have the PDB enabled by default even though you lower the replicas to 1.
     pdb:
       enabled: true
-      minAvailable: 1
+      maxUnavailable: 1

Makes sense! Thanks for the explanation :)
I want to run just one scheduler per cluster, since they'll all be configured the same! That's unrelated to this though.

Yeah it may be a bit overkill to have user-scheduler run with two replicas. They use leader-election so in practice it is only one doing the work and the other is standing by to do work if the other fails I think.

The new defaults for z2jh are the right thing to do now. Thanks for pushing this thorugh, @consideRatio!

It's a pleasure working with you @yuvipanda, I'm very thankful for your communication skills!


Improve autosuggest with module exports

VSCode Version:  1.11.2
OS Version: Win 8.1 64

Steps to Reproduce:

Create ES6 module with named export(s)
Import it

Intellisense suggests almost everything:

Exports are recognized:

It would be great if intellisense were suggesting only relevant exports or at least move them to the top of the list.

@tSte What is you configuration value for "editor.snippetSuggestions"? I looks to me that it's set to always sort snippets atop?

@jrieken it is inline. I know that I can leave it to bottom, but... I want to use inline everywhere else. It doesn't make sense to suggest this inside of import { ... } statements.

Yes, but our snippets aren't contextual (unfortunately). We don't have much information when inserting them since they are computed statically. We have three well known scopes: code, comments and strings. Making snippets aware of that is tracked here: #22661 but I am afraid it won't help you.
Because we have smart snippets we recommend to use extensions that are more contextual and can decide if a position is good for snippets. Closing as question because there is not much we can do. Thanks for understanding and Happy Coding


Ambiguity with tf.nn.embedding_lookup_sparse

Please make sure that this is a feature request. As per our GitHub Policy, we only address code/doc bugs, performance issues, feature requests and build/installation issues on GitHub. tag:feature_template
System information

TensorFlow 1.9.0
Python 3.6.6
Are you willing to contribute it (Yes/No): Yes

Describe the feature and the current behavior/state.
The tf.nn.embedding_lookup_sparse need two tf.SparseTensor (sp_ids and sp_weights) but I can't figure out why. Specifically, it seems to me that the columns of the indices of sp_ids and sp_weights are useless (see the example in the doc). This could be changed to use only one SparseTensor with:

rows of indices pointing to the index of the element of the batch.
columns of indices pointing to the index of the target element of the embedding (params).
values corresponding to the weights.

Small example of two operations doing the same things:
tf.sparse_tensor_dense_matmul(sparse_tensor, embeddings)
tf.nn.embedding_lookup_sparse(embeddings, sparse_ids, sparse_weights, "sum")
Here sparse_ids and sparse_weights have the same indices, the values of sparse_ids are the columns of indices of sparse_tensor and the values of weights are the values of  sparse_tensor. The three sparse tensors share the same rows of indices.
ps: It might not work in higher dimensional embedding lookups, I just wanted to point out the fact that embedding_lookup and embedding_lookup_sparse, despite their close names, have actually a very different usability.
Will this change the current api? How?
I can't really say.  tf.nn.embedding_lookup_sparse(params, sp_ids, sp_weights...) could become tf.nn.embedding_lookup_sparse(params, sparse_ids...) to be more consistent with tf.nn.embedding_lookup.
Who will benefit with this feature?
I don't really know.
Any Other info.
Have I written custom code: no
OS Platform and Distribution: linux, but not relevant
TensorFlow installed from: pip
TensorFlow version: 1.9.0
Bazel version: ?
CUDA/cuDNN version: no CUDA
GPU model and memory: no GPU
Exact command to reproduce: See above
Mobile device: not relevant

Thank you for your post. We noticed you have not filled out the following field in the issue template. Could you update them if they are relevant in your case, or leave them as N/A? Thanks.
Have I written custom code
OS Platform and Distribution
TensorFlow installed from
TensorFlow version
Bazel version
CUDA/cuDNN version
GPU model and memory
Exact command to reproduce
Mobile device

@brochier  - Hi, could you please provide the information asked ?

@drpngx  -  PTAL

Both fields of sp_ids are useful. It is assumed that sp_weights is "aligned" with sp_ids in the sense that sp_ids.indices == sp_weights.indices. Only sp_ids.indices[:, 0] (the first dimension) is used.


100 max revisions avail to download?

It appears so (at least w/free svc):
oxly clone --nrevs 101 dropbox://orgzly/misc-notes-winter17.org
Moving/saving old /tmp/oxnotes/.oxly/.tmp to /tmp/oxnotes/.oxly/.old/oxlytmp.22168 ... done.
Downloading metadata of 101 latest revisions on Dropbox ...
 Call to Dropbox to list file revisions failed: 101 is not within range [1, 100]

So we should prolly add this to doc and maybe s/50/100 as default recommended num.

README:
oxmerge example run
oxmerge dropbox://orgzly/misc-notes-spring17.org
oxly, version 0.9.21
Cloning dropbox://orgzly/misc-notes-spring17.org into /tmp/oxnotes ...
Moving/saving old /tmp/oxnotes/.oxly/.tmp to /tmp/oxnotes/.oxly/.old/oxlytmp.10636 ... done.
Downloading metadata of 50 latest revisions on Dropbox ... done.
--so s/50/100

The fix just warns clone users if nrevs>100.  Did'nt want hard limit as maybe non-free svc does not have same 100 max.
Also ups default nrevs to 100 from 50 so increases chance of a hit from the git-go (and subsequent clone will use local cached ones anyways) .


Make workspace client also return runtime dbutils when in dbr

Changes

workspace_client.dbutils always returned oss implementation of dbutils. We want it to also use dbr implementation when in dbr.

Tests

[x] Manually Test in dbr
[x] Test locally

[x] make test run locally
[x] make fmt applied
[ ] relevant integration tests applied

Suggestion: let's not include internal ticket IDs in PR descriptions in public repos.

Codecov Report
Patch coverage: 75.00% and no project coverage change.

Comparison is base (c9f7c41) 53.23% compared to head (3870318) 53.24%.

:exclamation: Current head 3870318 differs from pull request most recent head 6436051. Consider uploading reports for the commit 6436051 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #210   +/-   ##
=======================================
  Coverage   53.23%   53.24%
=======================================
  Files          32       32
  Lines       19138    19145    +7
=======================================
+ Hits        10188    10193    +5
- Misses       8950     8952    +2

Impacted Files
Coverage Δ

databricks/sdk/__init__.py
73.94% <75.00%> (-0.16%)
:arrow_down:

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.