Security for Metrics Endpoint

Env vars added:

METRICS_DISABLE_ENDPOINT  # default: false
METRICS_SECURE_ENDPOINT  # default: false
METRICS_API_KEY  # default: false
METRICS_MINIMUM_AUTHORITY_LEVEL  # default: false

Add API_KEY authority (including db migration)
Add metrics to config service
Add metrics guard

throw 404 if endpoint is disabled
if endpoint is secured,

if there's a logged-in user, check if they have the minimum required authority
if not, try to retrieve the api key and use the key to retrieve the user

if there's a user, use that to check if they have the minimum required authority
if there's no user, check if the api key is the same as the configured allowed key

checking the minimum required authority checks if the user has the authority defined in METRICS_MINIMUM_AUTHORITY_LEVEL, a greater authority, or the API_KEY authority

if endpoint is NOT secured, allow access

api-key.dao.ts: add dao function to check if the api key exists
Command module:

Add metrics key to the helm setup command
Add method & command to create an api key

LoggedInUserFactory.ts: fix spacing (no other changes)

@kyle-intelletive -

I didn't commit it because I didn't want to add to the migration tree yet.
See the comment in the guard. I think we could move that functionality into the logged-in user population file. It would allow us to pass in API keys as bearer tokens or headers. Beyond that, it's another programmatic API Key. Those are handled differently than users' individual API keys (which are attached to their permissions). I want to open access to the endpoint to different classes of "users":

ones with defined access levels who are logged in
ones with defined access levels who are not logged in but are using individual API keys (granted via the ui)
services that are using their assigned API key
services that are using the global API key

@kyle-intelletive

Moved more validation from the guard to the LOGGED_IN_USER provider

LOGGED_IN_USER provider can now get api keys from bearer tokens and headers

Authority.METRICS instead of Authority.API_KEY
Checked in the migration
Added a note to "save kubehunter report" to utilize the LOGGED_IN_USER provider

Any more thoughts on how it can be simplified/genericized?

@kyle-intelletive

  * LOGGED_IN_USER provider can now get api keys from bearer tokens and headers

* Authority.METRICS instead of Authority.API_KEY

* Checked in the migration

* Added a note to "save kubehunter report" to utilize the LOGGED_IN_USER provider

Any more thoughts on how it can be simplified/genericized?
Given that the LOGGED_IN_USER is factory, it will be kind of difficult

I can think of a couple ways. Here are some quick overviews of them, I can discuss them in more detail at some point if you'd like.
A. Change to service
If we use a service rather than factory (still scoped to request) - we could implement a decorator like below that would basically just call that service with the appropriate config. then we'd change our references to call this.LOGGED_IN_USER.get() rather than it being a defined variable. We'd also have to ensure that the Authentication() decorator is resolved before any other decorators that care about the user
B. Make mono-decorator
Basically, we roll the functionality of UseGaurds, AllowedAuthorityLevels, and LOGGED_IN_USER into one decorator.
then, we'd also have to define another decorator that functions like the @Body() decorator, that pulls the value of the user from the metadata that the main decorator will call. (Won't need that secondary decorator if we only care that the user is authenticated and not who they are)
C. Provide variations of the LOGGED_IN_USER
We can inject variations of the LOGGED_IN_USER factory that handle auth differently. Off the top of my head, I know we can do this at a module level, we'd have to do more research to see if it can be changed at the class/function level
Sample code for both A & B
@Authentication({
 authorityLevels: [Authority.Admin ...]
  // If true, checks for bearer token as "normal user"
  allowUsers: true,
  // If true, checks for API Key users
  allowApiKeys: true,
  // optional, checks some reasonable default if not set
  apiKeySources: {
     // Checks in the headers and query params of provided names
     headers: ['x-api-key', 'whatever-else' .....]
     queryParams: ['key', 'apiKey' ....],
     // Check if bearer token is actually an api key
     bearerToken: true
  }
})
myEndpoint() {
...
}

@kyle-intelletive
I can think of a couple ways. Here are some quick overviews of them, I can discuss them in more detail at some point if you'd like.
...
That seems like an expansion of scope to improved the way that the app fills/handles LOGGED_IN_USER (and subsequent authorization). Do you think this would make more sense as a new ticket?

@kyle-intelletive
I can think of a couple ways. Here are some quick overviews of them, I can discuss them in more detail at some point if you'd like.
...
That seems like an expansion of scope to improved the way that the app fills/handles LOGGED_IN_USER (and subsequent authorization). Do you think this would make more sense as a new ticket?

@becky-intelletive
For sure, this would definitely be a different feature. I will review the changes made from the other topics shortly.

@becky-intelletive For sure, this would definitely be a different feature. I will review the changes made from the other topics shortly.

https://dev.azure.com/intelletive/Minesweeper/_boards/board/t/Minesweeper Team/Issues/?workitem=6897

@kyle-intelletive - ready for re-review


liccheck vs liccheck2

What is the difference between those two:

https://pypi.org/project/liccheck/
https://pypi.org/project/liccheck2/

liccheck2 is a fork made to publish a version on pypi before we did it ourselves: https://github.com/graingert/python-license-check. So I guess it is obsolete now.

As you are the maintainer, could you delete liccheck2 (or at least write a notice / redirect there, that liccheck should be used)?

I am not the maintainer of liccheck2, @graingert is.

liccheck2 has a wheel, I'll make liccheck2 depend on liccheck when it has a wheel


Error downloading hunter X hunter

anime-dl.exe -i "http://www.crunchyroll.com/hunter-x-hunter/episode-1-departure-x-and-x-friends-584886" -u "" -p "" -r "1080p"
Trying to login...
Logged in successfully...
Grabbing Links for 1080p Streams.
Writing subtitles to files...
Writing subtitles to files...
Writing subtitles to files...
Writing subtitles to files...
Traceback (most recent call last):
File "anime_dl_main_.py", line 16, in
File "anime_dl_main_.py", line 56, in main
File "anime_dl\AnimeDL.py", line 27, in init
File "anime_dl\sites\crunchyroll.py", line 38, in init
File "anime_dl\sites\crunchyroll.py", line 167, in singleEpisode
File "site-packages\requests\sessions.py", line 501, in get
File "site-packages\cfscrape_init_.py", line 39, in request
File "site-packages\requests\sessions.py", line 488, in request
File "site-packages\requests\sessions.py", line 603, in send
File "site-packages\requests\sessions.py", line 685, in get_adapter
requests.exceptions.InvalidSchema: No connection adapters were found for 'mp4:c6/s/ve762525/video.mp4'
Failed to execute script main

Fixed. Please re-check the ReadMe's Dependencies section for more information on how to get around this issue.


Gladys fails to start using mongodb

So far I've followed the instructions according to README. I get Gladys run perfectly with mysql settings. But when I run node app.js with mongodb settings it fails.
Last commit: 98ea1cf6762cedc8ef6dccefe4c68b57aa6f963e
Mongodb settings:
glnv: {
    adapter: 'sails-mongo',
    host: 'localhost',
    port: 27017,
    // user: 'username',
    // password: 'password',
    database: 'glnv'
  },

Database and collections are being created after running node app.js
Error messages:
node app.js
info: User hook policies loaded from /home/vagrant/glnv/api/hooks/example/policies.
info: User hook config loaded from /home/vagrant/glnv/api/hooks/example/config.
info: User hook models loaded from /home/vagrant/glnv/api/hooks/example/lib/../models.
info: User hook services loaded from /home/vagrant/glnv/api/hooks/example/lib/../services.
info: User hook controllers loaded from /home/vagrant/glnv/api/hooks/example/lib/../models.
info:
info:                .-..-.
info:
info:    Sails              <|    .-..-.
info:    v0.11.0             |\
info:                       /|.\
info:                      / || \
info:                    ,'  |'  \
info:                 .-'.-==|/_--'
info:                 `--'-------'
info:    __---___--___---___--___---___--___
info:  ____---___--___---___--___---___--___-__
info:
info: Server lifted in `/home/vagrant/glnv`
info: To see your app, visit http://localhost:1337
info: To shut down Sails, press <CTRL> + C at any time.

debug: --------------------------------------------------------
debug: :: Sun Jun 14 2015 03:29:02 GMT+0000 (UTC)

debug: Environment : development
debug: Port        : 1337
debug: --------------------------------------------------------

/home/vagrant/glnv/node_modules/sails-mongo/node_modules/mongodb/lib/utils.js:97
    process.nextTick(function() { throw err; });
                                        ^
TypeError: Object #<bound> has no method 'query'
    at Object.module.exports.scheduleAllAlarms (/home/vagrant/glnv/api/services/AlarmService.js:134:9)
    at Object.bound [as scheduleAllAlarms] (/home/vagrant/glnv/node_modules/sails/node_modules/lodash/dist/lodash.js:729:21)
    at Object.module.exports.onStart (/home/vagrant/glnv/api/services/StartService.js:29:16)
    at Object.bound [as onStart] (/home/vagrant/glnv/node_modules/sails/node_modules/lodash/dist/lodash.js:729:21)
    at /home/vagrant/glnv/config/bootstrap.js:19:18
    at wrapper (/home/vagrant/glnv/node_modules/sails/node_modules/waterline/node_modules/lodash/index.js:3602:19)
    at applyInOriginalCtx (/home/vagrant/glnv/node_modules/sails/node_modules/waterline/lib/waterline/utils/normalize.js:421:80)
    at wrappedCallback (/home/vagrant/glnv/node_modules/sails/node_modules/waterline/lib/waterline/utils/normalize.js:320:18)
    at _normalizeCallback.callback.success (/home/vagrant/glnv/node_modules/sails/node_modules/waterline/node_modules/switchback/lib/normalize.js:33:31)
    at _switch (/home/vagrant/glnv/node_modules/sails/node_modules/waterline/node_modules/switchback/lib/factory.js:48:28)
    at /home/vagrant/glnv/node_modules/sails/node_modules/waterline/lib/waterline/query/dql/destroy.js:123:11
    at /home/vagrant/glnv/node_modules/sails/node_modules/waterline/node_modules/async/lib/async.js:189:25
    at bound.<anonymous> (/home/vagrant/glnv/node_modules/sails/node_modules/waterline/lib/waterline/utils/schema.js:151:44)
    at fn (/home/vagrant/glnv/node_modules/sails/node_modules/waterline/lib/waterline/utils/callbacksRunner.js:136:10)
    at iterate (/home/vagrant/glnv/node_modules/sails/node_modules/waterline/node_modules/async/lib/async.js:181:13)
    at Object.async.eachSeries (/home/vagrant/glnv/node_modules/sails/node_modules/waterline/node_modules/async/lib/async.js:197:9)

That's completely normal, Gladys is built to run with MySQL. Sometimes Gladys needs to perform very specific request that shouldn't be done with sailjs ORM, so it's done by doing MySQL raw queries (for performance).
And in fact Gladys couldn't be built with MongoDB, because the data Model has a lot of relationships, the data model need a relational database :) ( for more informations why MongoDB is not adapted sometimes : http://www.sarahmei.com/blog/2013/11/11/why-you-should-never-use-mongodb/comment-page-1/ )
But you are completely right, I should remove the defaults MongoDB connections lines in the config/connections.js folder, it was generated by sailsjs by default :)
Thanks for the feedback !


[web-sys] add setConfiguration to RtcPeerConnection

This PR adds setConfiguration to RTCPeerConnection object.
https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/setConfiguration

Missing a changelog entry.
Otherwise looks good, thank you!

Thank you for your review! Updated changelog and regenerated bindings


chore: update for polars 39

Do not call get(0) without checking if the chunk is empty

@morenol we should also apply this to preview-9


EACCESS crashes the server

I was instructed to file this upstream from https://github.com/vuejs/language-tools/issues/3511
My project directory contains a directory which is not readable (owned by a different user). The existence of this directory crashes the server.
Steps to reproduce

Install VS Cod(e|ium).
Install the Vue.volar plugin.
Open a Vue project
sudo mkdir problem

Expected behavior
The plugin ignores this error, perhaps emitting a warning, and continues to start. Alternatively, the plugin is configurable in some way to ignore certain files or directories within a project directory.
Error Logs
node:fs:1442
  handleErrorFromBinding(ctx);
  ^

Error: EACCES: permission denied, scandir '/home/scott/src/kalkutago/db.mount'
    at Object.readdirSync (node:fs:1442:3)
    at t.readdirSync (node:electron/js2c/asar_bundle:2:10837)
    at Object.readDirectory (/nix/store/nrwr1bw6hdg4k34can4y4qig73x9fi98-vscode-extension-Vue-volar-1.7.11/share/vscode/extensions/Vue.volar/dist/server.js:77:33947)
    at Object.readDirectory (/nix/store/nrwr1bw6hdg4k34can4y4qig73x9fi98-vscode-extension-Vue-volar-1.7.11/share/vscode/extensions/Vue.volar/dist/server.js:77:30107)
    at b (/nix/store/nrwr1bw6hdg4k34can4y4qig73x9fi98-vscode-extension-Vue-volar-1.7.11/share/vscode/extensions/Vue.volar/dist/server.js:77:3184)
    at /nix/store/nrwr1bw6hdg4k34can4y4qig73x9fi98-vscode-extension-Vue-volar-1.7.11/share/vscode/extensions/Vue.volar/dist/server.js:77:1942
    at w (/nix/store/nrwr1bw6hdg4k34can4y4qig73x9fi98-vscode-extension-Vue-volar-1.7.11/share/vscode/extensions/Vue.volar/dist/server.js:76:38771)
    at w (/nix/store/nrwr1bw6hdg4k34can4y4qig73x9fi98-vscode-extension-Vue-volar-1.7.11/share/vscode/extensions/Vue.volar/dist/server.js:76:39220)
    at Aoe (/nix/store/nrwr1bw6hdg4k34can4y4qig73x9fi98-vscode-extension-Vue-volar-1.7.11/share/vscode/extensions/Vue.volar/dist/server.js:76:38625)
    at Proxy.w (/nix/store/nrwr1bw6hdg4k34can4y4qig73x9fi98-vscode-extension-Vue-volar-1.7.11/share/vscode/extensions/Vue.volar/dist/server.js:77:1885) {
  errno: -13,
  syscall: 'scandir',
  code: 'EACCES',
  path: '/home/scott/src/kalkutago/db.mount'
}

Thanks for the report! It should be fixed by https://github.com/volarjs/volar.js/commit/327912a4c00947a5184444a97421f9bf4b89ef88 and released with 1.8.0, please make sure updated the extension version.

Thanks!


GO:0042823 pyridoxal phosphate biosynthetic process parentage query

If
GO:0042823 pyridoxal phosphate biosynthetic process
is
The chemical reactions and pathways resulting in the formation of pyridoxal phosphate, pyridoxal phosphorylated at the hydroxymethyl group of C-5, the active form of vitamin B6.
should it be part of
GO:0042819
Name	vitamin B6 biosynthetic process
The chemical reactions and pathways resulting in the formation of any of the vitamin B6 compounds; pyridoxal, pyridoxamine and pyridoxine and the active form, pyridoxal phosphate.
?

same question for GO:0008615

Hi @ValWood,
The issue here is due to this difference in GO vs ChEBI:
GO:
GO:0042823 pyridoxal phosphate biosynthetic process points to CHEBI:597326 pyridoxal 5'-phosphate(2-)
GO:0042819 vitamin B6 biosynthetic process points to CHEBI:27306 vitamin B6
ChEBI:
pyridoxal 5'-phosphate(2−) (CHEBI:597326) is conjugate base of pyridoxal 5'-phosphate (CHEBI:18405)
pyridoxal 5'-phosphate (CHEBI:18405) is a vitamin B6 phosphate (CHEBI:36970)
vitamin B6 phosphate (CHEBI:36970) has functional parent vitamin B6 (CHEBI:27306)
so vitamin B6 phosphate is a derivative of vitamin B6
Not sure how to fix this. Would you like to open a ticket with ChEBI and link it here please?
Thanks,
Paola

I think that's one for the editors, I have done my bit ;)  remember I'm a literature curator not an ontology editor....  and I'm not funded by GO.....

Sure @ValWood, I didn’t mean that ;-) It’s just that it looks more of an issue with ChEBI than with us, that’s why I’m not sure how to fix it. I’ll look into it some more. Cheers.

Yeah I'm not sure what the issue is, only that there is a problem which is filtering through to PomBase annotation consistency. I'm only reporting an issue. I'll help where I can but  I don't really know about how GO aligns with CHEBI....or what would be correct.

No worries @ValWood , I'll investigate and will post updates here.

Note for self: same as GO, ChEBI defines pyridoxal 5'-phosphate as “the active form of vitamin B6”
(see https://www.ebi.ac.uk/chebi/searchId.do;jsessionid=976E8823A58FC29140E69DBD51760FAC?chebiId=CHEBI%3A18405).
Therefore, it seems to me that, in order to achieve consistency, we need to consult with ChEBI about how they’d like to fix the discrepancy between pyridoxal 5'-phosphate being called a type of vitamin B6 and its derivative at the same time. Perhaps pyridoxal 5'-phosphate needs to be textually defined as “the active derivative of vitamin B6”. If so, we’d need ChEBI to create ‘vitamin B6 derivative’ or similar (and ‘vitamin B6 phosphate’ placed under it), and then GO would need to create ‘vitamin B6 derivative metabolic process’ or similar (and ‘GO:0042823 pyridoxal phosphate biosynthetic process’ would sit under it).

according to wikepedia
Vitamin B6 refers to a group of chemically very similar compounds which can be interconverted in biological systems.
so its really a grouping term rather than a specific compound if this is true...
I think i'd expect all to  be grouped as "vitamin B6 metabolism"

That would be my preferred solution too :-)

Me too.

Hi @ValWood , I opened a ChEBI ticket to see if they'd consider making vitamin B6 a grouping term.
We could then fix GO accordingly.
Setting to pending while I wait to hear from them.

Hi @ValWood ,
Update: I followed up with ChEBI today.

Hi @ValWood ,
FYI, I followed up with ChEBI on this one by email today.

Note for self: Gareth Owen from ChEBI emailed:
"I think the B6 problem is an annotation issue rather than a bug - we will try and get it sorted as soon as we can."

@ValWood
Steve from ChEBI writes:
“I've changed the 'vitamin B6 phosphate (CHEBI:36970) has functional parent vitamin B6 (CHEBI:27306)'
to vitamin B6 phosphate (CHEBI:36970) is a vitamin B6 (CHEBI:27306), as the phosphates are also biologically active forms of vitamin B6. So now the term 'vitamin B6' captures the parent compounds (pyridoxal, pyridoxamine, pyridoxic acid and pyridoxine) and their phosphates as a subclass (the individual phosphates have functional parent links to the individual parents).”
The change was made on Nov 21st (see http://www.ebi.ac.uk/chebi/searchId.do?chebiId=CHEBI:36970), but we’ll have to wait for the new ChEBI release (presumably around the beginning of Dec) for the change to percolate to GO. Then the parentage of
GO:0042823 pyridoxal phosphate biosynthetic process
and
GO:0008615 pyridoxine biosynthetic process
will be fixed automatically and linked to vitamin B6. Closing now.


MobileServiceSyncTable method PurgeAsync with Force flag

I need to purge entries on the client-side (mobile device) where there is a pending operation. To do that I need to call a version of PurgeAsync that accepts the "force" flag. There is no version available that takes a LINQ Where clause (IMobileServiceTableQuery) as parameter. The only one available for my purposes is one that takes a normal SQL string as the query/selection parameter. It seems like a step backwards to me to have to start writing my LINQ queries as plain-text now. Please consider one of the following as a solution to this problem:
Option 1: Expose a new version of PurgeAsync that takes an IMobileServiceTableQuery as well as the force flag as parameters
Option 2: Expose the following internal method of class MobileServiceTableQueryProvider to the outside world so that one can call it to prepare a plain-text string version of your query from your Linq query - this is already being used internally in the mobile sync client-side code so there is no reason that it could not be exposed quickly and easily: the method that will need to be exposed is: "internal string ToODataString(IMobileServiceTableQuery query)"
PS: This is how the above ToODataString() method is currently already being used in internal class MobileServiceSyncTable. If you go for option 2 above then this is what I would like to have access to from my own code i.e. from outside of the Mobile Services library:
public Task PurgeAsync<U>(string queryId, IMobileServiceTableQuery<U> query, CancellationToken cancellationToken)
        {
            if (query == null)
            {
                throw new ArgumentNullException("query");
            }
            string queryString = this.queryProvider.ToODataString(query);

            return this.PurgeAsync(queryId, queryString, false, cancellationToken);
        }

Can you give us a use case outside of the code.  What are you actually trying to achieve with this functionality?  (I understand the functionality - I want to understand the reasoning behind it)

I simply want to delete a record/object that has pending operations on it. There are currently six versions of the PugeAsync() method available for public use, of which two take the force flag. So my reasons are the same as the use cases for which the force flag is currently already available (i.e. nothing new). But the only problem with the existing versions that already have the force flag is that the first of the two existing versions wipes all the data in the table so that won't work for me, and the second of the two existing versions only takes a query expressed as a string. Hence my request.

Hi Adrian, I've made some changes to my local copy of this github project and it seems that it's working (with some changes to class PurgeAction method HandleDirtyTable because currently it only supports deleting ALL operations against a local table and it's ignoring the filter i.e./e.g. the object ID that's being passed in - amongst some other small changes). But before I submit it for your review to be merged let me give you my exact use case.
Step 1: I have some custom logic that pulls some data (which represents let's say an Agency server-side object) into memory of the local device from the server before giving the user the choice whether they want to save it locally or not. I get the server data with a PullAsync call to the server. Then, if the user decides to save the data locally, I use UpdateAsync() to save the data locally. Why do I do that? Because, firstly, using InsertAsync() locally with the data in memory would cause a new instance of the object to be created on the server, which is not the intention. And secondly because I don't want to make another PullAsync() call over the network just to save this data locally, seeing that it is already present in my local memory - so such a resource intensive call seems completely unnecessary.
Step 2: The above approach that I'm currently using i.e. using UpdateAsync() instead of PullAsync() seems to work fine in other aspects, except for when the user decides to delete the object from the local store before a PushAsync() has occurred. In that case, when PurgeAsync() is called, an exception is thrown because there is still a pending operation on the object that I inserted in the local store in step 1 above. That's why I need a Force flag on the PurgeAsync() method (the force flag would allow me to delete the local object despite the fact that there is a pending "update" operation for it).
So my primary question at this point is this: does the rationale that I set out in step 1 above hold up? Is it acceptable to call UpdateAsync() in this way or is there a fundamental reason why PullAsync() i.e. a seemingly unnecessary second round-trip to the server is required to safely store the object locally? (if the only problem with this approach is the local purging part that I described in part 2 above then I can submit my changes to my local project to get this working for your review).

This a duplicate of https://github.com/Azure/azure-mobile-apps-net-client/issues/56.


Volume and seek methods for user UPDATE

It didn't look like anything was happening with the volume and seek method PR created by vanderlin so I've added the optional device_id as requested.
Also rebased on current master.
@vanderlin @JMPerez @thelinmichael

I'm closing this PR. I have decided to create a PR combining the contributions from several pending PRs that tried to include the functionality but were either missing functionality (eg optional parameters) or tests.
Thank you very much for your contribution and we¡ll try to provide feedback and act on this earlier next time.


Content missing Azure PowerShell steps

This page is missing the steps using Azure PowerShell cmdlets
cc: @Alex-wdy

Document Details
⚠ Do not edit this section. It is required for learn.microsoft.com ➟ GitHub issue linking.

ID: 298b87f4-b6eb-5642-7ab5-cd6df709390d
Version Independent ID: 0518cce4-7677-6d63-979d-e0a9e532efc0
Content: Quickstart: Create a Node.js web app - Azure App Service
Content Source: articles/app-service/quickstart-nodejs.md
Service: app-service
GitHub Login: @msangapu-msft
Microsoft Alias: msangapu

@dcaro thanks for bringing this to our attention. We have assigned this issue to the author to review further and take the next course of action.

#needs-sme-input


[metrics] update push metrics to leverage prometheus counters

These already found their way into the logger. We don't have bandwidth
to update all the code to provide the labeled counters for TCBs and also
fix the use of logging counters, so might as well adapt.

Verified in cluster test!

/land


pip_audit, test: allow editable packages to be skipped

WIP; needs https://github.com/di/pip-api/pull/131 to support requirements files and not just environments.
Closes #237.

Pending on release: https://github.com/di/pip-api/pull/132

Released: https://pypi.org/project/pip-api/0.0.28/


chore(deps): bump https://github.com/zeebe-io/zeebe-cluster-helm from 0.0.101 to 0.0.102

Update zeebe-io/zeebe-cluster-helm from 0.0.101 to 0.0.102
Command run was jx step create pr chart --name zeebe-cluster --version 0.0.102 --repo https://github.com/zeebe-io/zeebe-full-helm.git

[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: salaboy
If they are not already assigned, you can assign the PR to them by writing /assign @salaboy in a comment when ready.
The full list of commands accepted by this bot can be found here.
The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment


Remove dependency installation from project configs

Goals
Remove dependency installation from project config files.
The changes to install requirements per project in the scheduled BuildKitten job can be found in https://github.com/explosion/buildkite-pipelines/pull/9.
Description

Remove dependency installation steps/commands from project config files.
Add removal of project dependencies in GH CI after test suite for project has finished.

Types of change

Refactoring.
Checklist

[x] I confirm that I have the right to submit this contribution under the project's MIT license.
[x] I ran the tests, and all new and existing tests passed.
[x] My changes don't require a change to the documentation, or if they do, I've added all required information.

Sofie probably wants to weigh in on this in general.
I'm not sure why the CI isn't triggered, though? Let me try closing and reopening...

So the Azure CI runs succeed, but the BuildKite runs don't. I think this is due to some version mismatches that are a consequence of not being able to properly uninstall all packages. So two possibilities here:

I try to patch this particular issue up and hope it was the last one.
I set up an alternative version with a separate venv for each project.

I'm in favor of the latter, since it's easier to maintain. If the extra installation time is that horrid, we can still go back to the progressive uninstall/install way of setting the projects up. What do you think?

The underlying reason is that I made a change in spacy v3.2.0 that broke the compatibility (which I shouldn't have), so if you get spacy v3.1 and spacy-experimental than you can get this error.
I think the solution is make sure spacy-experimental is uninstalled? Are you sure the install step is working correctly? You can see traces of it here:
https://buildkite.com/explosion-ai/spacy-projects/builds/77#0182251c-b49d-4f78-af86-afb7a9ee4e3d/664-1017

Are you sure the install step is working correctly?

IMO the install step is working fine, it's the pip uninstall that's failing. Looking into it.

I meant uninstall, sorry.

I meant uninstall, sorry.

Using a venv on BuildKite and a fix for the spaCy re-installation w.r.t. the project config files seems to have resolved all issues.

This is looking pretty good! I like that fact that it's now just one script to maintain.

How is this going to affect users running existing projects end-to-end? They need to remember to install the requirements first?

I don't think we've decided for sure what to do here. This shows how a good plan for how we could manage the requirements for our CI/tests to make sure that the requirements are also maintained, since having a bunch of them in azure-pipelines.yml wasn't a particularly good way to do it.
For users I could imagine:

an automatic step in spacy project run that uses importlib to check whether the requirements are installed if requirements.txt is present and provides example install commands for users if not
an automatic script that adds/maintains install steps within the projects that we maintain here
???

I don't think we've decided for sure what to do here. ...

I like the idea of automatically checking if the requirements are installed + providing install commands or even having users confirm is the requirements should be installed automatically.
I think we could do that before running any step though (shouldn't take too long to do so?), not as a separate step.

I think we could do that before running any step though (shouldn't take too long to do so?), not as a separate step.

I'm not sure I fully understand what you mean, like if we do it before running other steps, it becomes a separate step, no? Don't mean to be pedantic, just confused 😛
I definitely agree that this solution feels better for the CI/test suite of things, but I want to make sure things don't unnecessarily break for users. We don't really version the projects or anything, but workflows that checkout a given project and run some kind of all command will suddenly break if the requirements aren't installed anymore as part of the workflow. So at the very least we need to think about how to communicate this clearly.
Do we want something like assets so we'd have
spacy project install
spacy project assets
spacy project run ...

or such?

* an automatic step in `spacy project run` that uses `importlib` to check whether the requirements are installed if `requirements.txt` is present and provides example install commands for users if not

I like that idea too, but it might generate some unnecessary overhead if this is run on every single spacy project run ... command?

I think we could do that before running any step though (shouldn't take too long to do so?), not as a separate step.

I'm not sure I fully understand what you mean, like if we do it before running other steps, it becomes a separate step, no? Don't mean to be pedantic, just confused stuck_out_tongue

Yeah, I misunderstood what Adriane was saying - I meant exactly what she's saying in her first suggestion :smile: We could draft something and run some tests to see how much overhead it really is? I feel like this is the safest and lowest-effort (for users) solution.
If this doesn't work out for whatever reason, I'm in favor of spacy project install. Feels appropriate.

I guess we want to try adding the requirements checking/automatic install (or, alternatively, spacy project requirements/install) to spaCy before we merge this PR then?

Alright, so checking whether just the packages in requirements.txt are installed seems to be not too much work and achievable with only pkg_resources. Getting the complete dependency tree is a bit more involved.
What we'd be looking for here, ideally, is something like pip check to ensure all packages are installed with the right versions. This works with conda as well. Haven't tried poetry or other package managers yet. Small hiccup: installing spaCy from scratch in a new conda environment and then running pip check yields:
thinc 8.0.15 has requirement pydantic!=1.8,!=1.8.1,<1.9.0,>=1.7.4, but you have pydantic 1.9.0.
spacy 3.3.1 has requirement pydantic!=1.8,!=1.8.1,<1.9.0,>=1.7.4, but you have pydantic 1.9.0.

So I guess we have to be somewhat lenient about version mismatches. In general I think the combination of

checking if all packages in requirements.txt are installed using pkg_utils and
ensure dependency tree correctness/completeness with pip check
might be a low-effort and relatively robust way, based only on standard tools, to check if everything is installed properly.

What do you think?

I would only check for the packages in requirements.txt and ignore the full dependency check.
It doesn't look like the dependencies in the conda packages are incorrect (it should be <1.9.0 for that version), so I'm not sure what's going on there.

Suggestion for dependency checking: https://github.com/explosion/spaCy/pull/11226. This doesn't install dependencies yet, just detects. Only with an automatic install we'd be fully backwards compatible, but I feel like that's kind of intrusive for a default behavior...

This should be restored to its original state before I skillfully erased all of it. I'll trigger the all tests on BuildKite - once these succeed, I'll update again asking for a quick once-over review.

BuildKite run succeeds. A final review/check appreciated, but there's no hurry since we don't wanna merge this before spaCy 3.5 is released anyway.

It might also make sense to change the spacy requirements in the CI.

To >=3.4?

Why is this change necessary?

Not doing that results in
ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
spacy-experimental 0.5.0 requires spacy<3.4.0,>=3.3.0, but you have spacy 3.4.2 which is incompatible.

Ah, then I would change it to:
spacy-experimental>=0.2.0,<0.7.0

@adrianeboyd @svlandeg This is ready to go IMO (a last sanity check might not hurt). The dependency check was already included in spaCy v3.4.3, so I think there's no need to wait for v3.5 to merge this.

The benchmarks/nel project seems to have unrelated changes?

The benchmarks/nel project seems to have unrelated changes?

That's true, but these are just formatting. Do you mind leaving them in? It's no issue to revert those either.

@adrianeboyd Do you feel the open issues have been addressed?

In this PR adding a correct spacy_version in nel_emerson would be okay, but if you're going to update the project code instead (which is fine!), please move these changes to a separate PR , and also add the correct spacy_version requirements.

In this PR adding a correct spacy_version in nel_emerson would be okay, but if you're going to update the project code instead (which is fine!), please move these changes to a separate PR, and also add the correct spacy_version requirements.

Was about to write a comment in this regard. In order for the nel_emerson  tests to succeed, we can either

limit spacy_versionto < 3.5 or
update KnowledgeBase to InMemoryLookupKB and limit spacy_version to >= 3.5 or
add qualified imports everywhere - something like:

try:
    from spacy.kb import InMemoryLookupKB as KnowledgeBase
except ModuleNotFoundError:
    from spacy.kb import KnowledgeBase

Inclined towards the first option. What do you think?

I realize that I don't think we highlighted how breaking the KnowledgeBase changes were in all the release notes.
For this PR, I think adding spacy_version: ">3.0.0,<=3.5.0" is fine. Any other options would be better as a separate PR (that would be merged before this one).

I realize that I don't think we highlighted how breaking the KnowledgeBase changes were in all the release notes.

Yeah, @svlandeg and me went over the issue of the KB changes being breaking a lot before merging it. I'm not super happy about this not being backward-compatible, but we didn't come up with a solution without drawbacks.

I realize that I don't think we highlighted how breaking the KnowledgeBase changes were in all the release notes.

You're right - should we go in and make that more explicit in the release notes?

I think it would be good to add it explicitly to the release notes and usage docs, yes.

Thanks for https://github.com/explosion/projects/pull/121/commits/3dc39b91f6d74aed85698277fd5f73e15b4ca884, @adrianeboyd! Was looking into it. Updated your change to reflect the new step name.

Ah, bad merge on my part...

(But I think that capture=True doesn't actually work correctly in general.)

Yay, I think this is ready to merge!


ViewMaster question

Not an issue, but question about the ViewMaster -- can you view arbitrary webpages and have accelerometer/head turns scroll the page? Thought it would be locked down to their content capsules...
Very cool project!

The View Master is just a nice implementation of the Google Cardboard spec (see https://www.google.com/get/cardboard/).
To use it for viewing web pages, you would need to display the page in the same sort of stereoscopic view that this project does. That might be doable with a browser plugin. Or a custom mobile browser (native app). If you get something running, I'd be interested to see it.
Glad you like the project!


Simplify and refine async context tracking

There are two key changes here:

Elimate the root context frame: It occurred to me that we really do not need a root AsyncContextFrame instance at all since there's nothing actually being stored there ever. Just have AsyncContextFrame::current() return an empty kj::Maybe in that case and treat the nullptr as equivalent to the root frame.
Makes use of an obscure v8 API that allows setting continuation context for promises and tasks. Allows a number of simplifications.

Also fixes a couple of minor bugs.

Ok, now that it's clear this is definitely the direction we want to go, I've merged the two PRs and cleaned up more of the bits that are made obsolete by relying on the v8 API.
Thenables are still a challenge here but there's a possible workaround using AsyncResource and we do expect v8 to fix that limitation in the future so I'm not that concerned.

@harrishancock ... given the substantial changes, I've cleared your ok. Can you give it another look over?

I can't figure out how to get a diff from what I've already reviewed to the current state of the PR. It seems like several additional changes have been rebased together with stuff I've reviewed already, and the whole thing was also rebased on master in the meantime too.

There are enough additional cleanups and a number of new commits here that it's worth looking at it as a fresh review.

looks good but needs squash

Squashed and rebased.


Prepare for External Testing in TestFlight

parent #521, parent #523, parent #535, parent #555

See #708


Max sum rectangle

Have you read the Contributing Guidelines on Pull Requests?
Yes
(Write your answer here.)
Description
This is Java code to find the maximum sum of a sub-matrix from a matrix, which has been done with the help of Kadane's algorithm  to reduce time complexity from O(n^4) to O(n^3)
(Write your answer here.)
Checklist

[x] I've read the contribution guidelines.
[x] I've checked the issue list before deciding what to submit.
[x] I've edited the README.md and link to my code.

Related Issues or Pull Requests
Issue #937
(Write your answer here.)

@iamrajiv @atarax665 @siddharth25pandey @Kajol-Kumari please review this