Drop "consume request on 401" behavior.

Resolves #11, #12, #27.
Production-quality WSGI servers such as Gunicorn and uWSGI handle this already. WSGI-Kerberos trying to handle it at the WSGI middleware layer defeats the safer and more robust handling that otherwise comes out-of-the-box in production quality WSGI servers.
The consume request behavior was probably originally added to WSGI-Kerberos when testing with a non-production-quality WSGI server, not realizing that simply using a production-quality WSGI server would have made clients' connection issues disappear, and in a safer and more efficient and performant way. /cc @mkomitee
Submitting this as a new PR based on discussion in #27 (after originally submitting this in #25; GitHub isn't letting me reopen #25 for some reason).
Having WSGI-Kerberos immediately send 401 responses without first waiting to drain the request socket for potentially naive clients allows production-quality WSGI servers (or even an upstream proxy like NGINX) to immediately see the 401, and then take responsibility for draining the request socket in a safe way before forwarding the 401 to the client.

This was already discussed before - _consume_request exists because without it, clients would get a broken pipe error on a large POST request that returns a 401 or 403. It doesn't matter what server you're using - it affects all of them (I've verified it with both gunicorn and uwsgi). Different web servers have different limits on how much of the data they would consume in-memory before streaming the input to the app, but once you go beyond that, the app handles streaming the data.

This was already discussed before - _consume_request exists because without it, clients would get a broken pipe error on a large POST request that returns a 401 or 403. It doesn't matter what server you're using - it affects all of them (I've verified it with both gunicorn and uwsgi). Different web servers have different limits on how much of the data they would consume in-memory before streaming the input to the app, but once you go beyond that, the app handles streaming the data.

Can you share how you're testing? I've also tested with Gunicorn against these changes, along with requests_kerberos (with force_preemptive=False) uploading 250 MB POST request body, and could not reproduce any ConnectionError. Note, if testing with Gunicorn out front (not behind a reverse proxy), you have to change its default "sync" worker to one of the async workers, as its docs prescribe. Is that maybe why you saw what you saw? If you try with e.g. gunicorn -k gthread --threads 10, do you still see the issue?

Can you share how you're testing? I've also tested with Gunicorn against these changes, along with requests_kerberos (with force_preemptive=False) uploading 250 MB POST request body, and could not reproduce any ConnectionError. Note, if testing with Gunicorn out front (not behind a reverse proxy), you have to change its default "sync" worker to one of the async workers, as its docs prescribe. Is that maybe why you saw what you saw? If you try with e.g. gunicorn -k gthread --threads 10, do you still see the issue?

Sure - I'm using the built-in example/example_application.py script in the repo with read_max_on_auth_fail=0 passed to KerberosAuthMiddleware.
Servers:
$ gunicorn --version
gunicorn (version 19.7.1)
$ gunicorn --log-level debug --bind ':8080' example_application:application

$ uwsgi --version
2.0.18
$ uwsgi --http :8080 --wsgi-file example/example_application.py

Client:
# not including requests_kerberos as I expect to get a 401 response
$ python -c 'import requests, r=requests.post("http://server:8080", files={"files": open("large-file", "rb")});print(r.text)'

ConnectionResetError: [Errno 104] Connection reset by peer
...
urllib3.exceptions.ProtocolError: ("Connection broken: ConnectionResetError(104, 'Connection reset by peer')", ConnectionResetError(104, 'Connection reset by peer'))
...
requests.exceptions.ChunkedEncodingError: ("Connection broken: ConnectionResetError(104, 'Connection reset by peer')", ConnectionResetError(104, 'Connection reset by peer'))

I tried with gunicorn -k gthread and couldn't reproduce this behavior. It's not really a solution though as the library should work for all cases.

Sure - I'm using the built-in example/example_application.py script in the repo with read_max_on_auth_fail=0 passed to KerberosAuthMiddleware.
Servers:
$ gunicorn --version
gunicorn (version 19.7.1)
$ gunicorn --log-level debug --bind ':8080' example_application:application

$ uwsgi --version
2.0.18
$ uwsgi --http :8080 --wsgi-file example/example_application.py

Client:
# not including requests_kerberos as I expect to get a 401 response
$ python -c 'import requests, r=requests.post("http://server:8080", files={"files": open("large-file", "rb")});print(r.text)'

ConnectionResetError: [Errno 104] Connection reset by peer
...
urllib3.exceptions.ProtocolError: ("Connection broken: ConnectionResetError(104, 'Connection reset by peer')", ConnectionResetError(104, 'Connection reset by peer'))
...
requests.exceptions.ChunkedEncodingError: ("Connection broken: ConnectionResetError(104, 'Connection reset by peer')", ConnectionResetError(104, 'Connection reset by peer'))

I tried with gunicorn -k gthread and couldn't reproduce this behavior. It's not really a solution though as the library should work for all cases.

I tried with gunicorn -k gthread and couldn't reproduce this behavior.

Aha! You were testing with the gunicorn sync worker with no reverse proxy in front of it. This is an invalid test. The Gunicorn docs expressly explain that this configuration defeats the production-quality protections built into Gunicorn. Please see https://docs.gunicorn.org/en/stable/design.html#choosing-a-worker-type ("If you exposed synchronous workers to the internet, a DOS attack would be trivial by creating a load that trickles data to the servers" etc.) for more information.

It's not really a solution though as the library should work for all cases.

This conclusion does not follow when you've tested with a known-bad configuration. As discussed, by WSGI-Kerberos making the server block indefinitely while it downloads the entire request before it even begins to send the 401 response, it defeats the safer and more sophisticated ability of a production-quality server to drain the socket asynchronously while it can continue other processing.

I tried with gunicorn -k gthread and couldn't reproduce this behavior.

Aha! You were testing with the gunicorn sync worker with no reverse proxy in front of it. This is an invalid test. The Gunicorn docs expressly explain that this configuration defeats the production-quality protections built into Gunicorn. Please see https://docs.gunicorn.org/en/stable/design.html#choosing-a-worker-type ("If you exposed synchronous workers to the internet, a DOS attack would be trivial by creating a load that trickles data to the servers" etc.) for more information.

It's not really a solution though as the library should work for all cases.

This conclusion does not follow when you've tested with a known-bad configuration. As discussed, by WSGI-Kerberos making the server block indefinitely while it downloads the entire request before it even begins to send the 401 response, it defeats the safer and more sophisticated ability of a production-quality server to drain the socket asynchronously while it can continue other processing.

See also https://werkzeug.palletsprojects.com/en/1.0.x/request_data/ (as I also linked in our previous discussion), e.g.

General rule: Leave the WSGI input stream alone. Especially in WSGI middlewares. Use either the parsing functions or the request object. Do not mix multiple WSGI utility libraries for form data parsing or anything else that works on the input stream.

See also https://werkzeug.palletsprojects.com/en/1.0.x/request_data/ (as I also linked in our previous discussion), e.g.

General rule: Leave the WSGI input stream alone. Especially in WSGI middlewares. Use either the parsing functions or the request object. Do not mix multiple WSGI utility libraries for form data parsing or anything else that works on the input stream.

I understand the recommendation to leave the wsgi input stream alone, but that's because it would interfere with other middlewares. That logic doesn't apply to wsgi_kerberos because the request is rejected outright in those cases
You could certainly recommend that users should use an async server, but we have no control or say into what people actually do. wsgi_kerberos should "just work" without requiring that users use specific web servers or specific configurations.

I understand the recommendation to leave the wsgi input stream alone, but that's because it would interfere with other middlewares. That logic doesn't apply to wsgi_kerberos because the request is rejected outright in those cases
You could certainly recommend that users should use an async server, but we have no control or say into what people actually do. wsgi_kerberos should "just work" without requiring that users use specific web servers or specific configurations.

I'm not sure that's a valid argument when it's based on using software in the opposite way to how the docs tell you to use it.  As long as WSGI-Kerberos is doing a blocking "consume request" before starting to send a 401, I'm concerned it doesn't "just work". And in a worse way, even: WSGI-Kerberos is making the server more vulnerable and less performant no matter what WSGI server, configuration, or proxy you use with it.
Can you find an example of other production-quality WSGI middleware that does a blocking "consume request" before it begins to send a response?

I'm not sure that's a valid argument when it's based on using software in the opposite way to how the docs tell you to use it.  As long as WSGI-Kerberos is doing a blocking "consume request" before starting to send a 401, I'm concerned it doesn't "just work". And in a worse way, even: WSGI-Kerberos is making the server more vulnerable and less performant no matter what WSGI server, configuration, or proxy you use with it.
Can you find an example of other production-quality WSGI middleware that does a blocking "consume request" before it begins to send a response?

The kerberos middleware is different from most other middleware, since it can return prematurely in addition to modifying the environment. There is also werkzeug.wsgi.LimitedStream.exhaust, which exists for this exact purpose. werkzeug.formparser uses it and I've seen a few other examples of it on the internet. In theory, we could just use that instead of implementing our own solution, although it does introduce another dependency.
More importantly, having a specific wsgi server or production configuration is not a requirement for using wsgi_kerberos. It should also work with a flask development server, for example. And by "work", I mean not getting broken pipe errors in the client (which users may or may not have control over).

The kerberos middleware is different from most other middleware, since it can return prematurely in addition to modifying the environment. There is also werkzeug.wsgi.LimitedStream.exhaust, which exists for this exact purpose. werkzeug.formparser uses it and I've seen a few other examples of it on the internet. In theory, we could just use that instead of implementing our own solution, although it does introduce another dependency.
More importantly, having a specific wsgi server or production configuration is not a requirement for using wsgi_kerberos. It should also work with a flask development server, for example. And by "work", I mean not getting broken pipe errors in the client (which users may or may not have control over).

I agree we shouldn't introduce a dependency on all of Werkzeug just for LimitedStream. (At some point it may be worth replacing the tests' dependency on WebTest with Werkzeug, but that can be taken up separately.)
I'm still not sure it's better to optimize WSGI-Kerberos's default behavior for misbehaving clients + development servers, but I'll close this given the lack of interest.

I agree we shouldn't introduce a dependency on all of Werkzeug just for LimitedStream. (At some point it may be worth replacing the tests' dependency on WebTest with Werkzeug, but that can be taken up separately.)
I'm still not sure it's better to optimize WSGI-Kerberos's default behavior for misbehaving clients + development servers, but I'll close this given the lack of interest.

Can you find an example of other production-quality WSGI middleware that does a blocking "consume request" before it begins to send a response?

Just wanted to say I tried to find some examples of this and couldn't. None of the other WSGI authentication middleware implementations I found consume requests on 401:

wsgi-basic-auth
wsgi-oauth2
django-adminrestrict

That said, I'm not sure how widely-used or production-quality these are; I had trouble finding any that looked obviously widely-used and production-quality.

Can you find an example of other production-quality WSGI middleware that does a blocking "consume request" before it begins to send a response?

Just wanted to say I tried to find some examples of this and couldn't. None of the other WSGI authentication middleware implementations I found consume requests on 401:

wsgi-basic-auth
wsgi-oauth2
django-adminrestrict

That said, I'm not sure how widely-used or production-quality these are; I had trouble finding any that looked obviously widely-used and production-quality.

wsgi_kerberos is unique in that it's the only one that has a challenge-response type workflow on each request. While uploading a file to an endpoint that requires basic auth may cause a broken pipe, if you didn't supply credentials, it would have failed anyway.

wsgi_kerberos is unique in that it's the only one that has a challenge-response type workflow on each request. While uploading a file to an endpoint that requires basic auth may cause a broken pipe, if you didn't supply credentials, it would have failed anyway.


Allow sort direction of "none" to be applied multiple times

Closes https://github.com/gregnb/mui-datatables/issues/966.

Coverage decreased (-0.04%) to 76.597% when pulling bdff5fa839f5632fd4c3ab3d77b307977fba66ef on bugfix/issue-966-sort-direction-removal into f0ea9e58d82839c57a4dfd4a342b6649932660a7 on master.

@gabrielliwerant , When this will change be available for us to use?

It should be available now in 2.12.0.


[acr] Make audience optional

Fixes #25167

API change check
APIView has identified API level changes in this PR and created following API reviews.
azure-container-registry


Update streaming example

Codecov Report

Merging #1361 into master will increase coverage by 0.45%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1361      +/-   ##
==========================================
+ Coverage   86.33%   86.79%   +0.45%
==========================================
  Files          57       57
  Lines        2174     2174
  Branches      111      111
==========================================
+ Hits         1877     1887      +10
+ Misses        297      287      -10

Impacted Files
Coverage Δ

...rc/main/scala/io/circe/shapes/SizedInstances.scala
78.57% <0%> (-14.29%)
:arrow_down:

.../core/shared/src/main/scala/io/circe/Decoder.scala
85.71% <0%> (ø)
:arrow_up:

...rc/main/scala/io/circe/numbers/BiggerDecimal.scala
85.96% <0%> (+0.58%)
:arrow_up:

...les/core/shared/src/main/scala/io/circe/Json.scala
80.48% <0%> (+2.43%)
:arrow_up:

...re/shared/src/main/scala/io/circe/KeyDecoder.scala
97.29% <0%> (+2.7%)
:arrow_up:

...re/shared/src/main/scala/io/circe/JsonNumber.scala
98.79% <0%> (+6.02%)
:arrow_up:

...d/src/main/scala/io/circe/NonEmptySeqDecoder.scala
100% <0%> (+8.33%)
:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ebb6d6...7bab4c4. Read the comment docs.


Update with baseline profile run against 6.12.1

Goal

Testing

Release Notes

WHAT:
WHY:

#1338  👈
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @bidetofevil and the rest of your teammates on  Graphite

Merge activity

Sep 9, 10:53 AM EDT: @bidetofevil started a stack merge that includes this pull request via Graphite.


GTFS examples

As I didn't cover everything in video, here are few other things I'm using.
For example, for individual tracking, I've created for each family member it's "in vicinity" sensor:

It will than create a bunch (if exist) stops for all transport types.
These can be used as map overlays:

Code for card looks as follows:
```
type: custom:auto-entities
card:
  type: map
  hours_to_show: 2
  dark_mode: true
  default_zoom: 17
  auto_fit: true
filter:
  include:
    - entity_id: '*_local_stop_person_andrej*'
    - entity_id: person.andrej
show_empty: true
```
Also, you can create flex table with data for everyone or just on person:

type: custom:auto-entities
filter:
  include:
    - name: '*local_stop*'
  exclude:
    - state: unavailable
card:
  type: custom:flex-table-card
  clickable: true
  max_rows: 30
  title:  ZET departures
  sort_by:
    - next_departures_lines
  columns:
    - hidden: true
      data: next_departures_lines
      modify: x.departure
    - name: Route
      data: next_departures_lines
      modify: '''<ha-icon icon='' + x.icon + ''></ha-icon>'' + x.route'
    - name: Time
      data: next_departures_lines
      modify: x.departure
    - name: Stop
      data: next_departures_lines
      modify: x.stop_name
    - name: Headsign
      data: next_departures_lines
      modify: x.headsign
  css:
    table+: 'padding: 0px'
  card_mod:
    style: |
      ha-card {
        overflow: auto;
        max-height: 500px;
      }

Thanks for this. However, I'm still not getting a return.
Am I missing something, the only element of the above script that should be changed is the 'local_stop'?

This is the attributes file;

This is the attributes file from the above sensor;

Thanks so much for this. I was editing the 'local_stop' to the sensor name and that was what was wrong. My bad.


Improve typing of normalizeRecipients

There's a TODO next to normalizeRecipients, so do it (fixing a minor bug in the process).
Also, add documentation for it. (The documentation is based on its original implementation and use in a2419662d431f2fe4f9a17f9f618371197fe1b20, the comment noted in the commit. Its use has expanded beyond that... although not necessarily for the better.)

Withdrawn pending analysis of test failures.

(Test failure was due to the mock Message in the test missing fields. Revised.)


Windows DomainSecurityScanner Pre-Complied Issues with Creating Output File

Hello - both the individual and bulk processing using the Windows pre-compiled DSS binary have been interactively returning console results fine so far.  However, when I attempt to write file ("-w") or redirect standard output (using ">"), can't seem to get it to work on Win10 Pro, tried on two systems.
The "-w" for the below produced a serialized *.yaml filename.  The second with output redirect ">" reported results interactively, put "INF Accepting input from STDIN.." in the file but still only reported results interactively. The testdin.csv file contains three entries in the form of:
domain1.com
domain2.com
domain3.com
Command line test use:
dss scan -t120 -w report.txt -n <IP_ADDRESS> < testdin.csv
dss scan -t120 -n <IP_ADDRESS> < testdin.csv > report.txt
Could you please review and advise?
Respectfully,
Orlando

Hey @OrlandoMD, thanks for taking the time to open this issue.
Your command should look like this:
dss scan -t120 -w -n <IP_ADDRESS> < testdin.csv

The -w flag doesn't accept a value after specifying it, as it's a simple boolean flag (whether to write to a file, or not). It simply writes the results to a file named after the current unix time at the time the command is run. I do believe that your syntax does make more sense. Perhaps it should be changed so that if -w is specified with an argument, it'll write to that file instead

@OrlandoMD I've reworked this behaviour to be a little clearer and to follow more common conventions regarding outputting results to a file.
Your command above should now look like this:
dss scan -t 120 -n <IP_ADDRESS> -o report < testdin.csv

This will output the domains to files like this:
report.yaml
report.1.yaml
report.2.yaml

I think it'd be nice to output multiple domains from the same session into the same file, but I'll revisit that

Hello - this helps!   I'm running the utility with the following batch file listed below to help aggregate results.
=========================
:  Domain Security Scanner Batch v0.1  8/15/2023
:  Orlando Stevenson - https://www.linkedin.com/in/orlandostevenson/
:
:  Loop to read file of domains an execute the windows64 compiled DSS utility
:  https://github.com/GlobalCyberAlliance/DomainSecurityScanner
:
:
@echo off
SETLOCAL ENABLEDELAYEDEXPANSION
:  Check if the file is provided
if "%~1"=="" (
echo Please provide list of domains file name
exit /b
)
IF EXIST merged-report.txt DEL /F merged-report.txt
:  For each line in the input file
for /F "tokens=*" %%A in (%~1) do (
IF EXIST report.yaml DEL /F report.yaml
echo %counter% - Working %%A ...
dss.exe scan %%A -t120 -n<IP_ADDRESS>,<IP_ADDRESS> -o report
type report.yaml >> merged-report.txt
)
ENDLOCAL
=========================
Using batch CLI  comment such as TYPE *.yaml > merged_report.txt could also be used.  I have over a 1000 so  I chose to loop and evaluate each one with its own DSS call.  Doing so has also helped in the past obtain more complete results.
Suggestion: The ability to use the STDOUT approach with ">" would be a helpful improvement.. perhaps eliminating the need for "-o" command line optionn.
Thank you for updating and enhancing this facility!
Orlando

Hey @OrlandoMD, I've fixed the ability to pipe the output directly :) While the current solution works well for most cases, I still don't like it for bulk usage (at runtime), for example:
dss scan globalcyberalliance.org gcatoolkit.org google.com github.com

So I may rework it to output an array, but I'm not sold on that idea yet. Hopefully the ability to correctly pipe solves your issues at least!


Run test scripts. after. the LZ2 upgrade

Describe the issue
After LZ2 upgrade, at least the Guardrails scanning test need to be run to check that nothing has changed
The other tests (for example, create a new account) have been test in the other LZ, so properly speaking we do not need to run.
Additional context

The test plan should have found no problems in LZ0 and LZ1 (scripts take 20m to run, 3 or 4 h to gather results, then check them over)
This test does not involve CheckPoint APIs, only AWS APIs

Definition of done

resolve the duplicate related ticket
Run the Guardrails scanning test and compare the results before/after the upgrade. The goal will show what has changed and also may help to improve the scripts showing areas that need to be better scanned and areas that perhaps do not need to be scanned

Done. Results can be found at
https://bcgov.sharepoint.com/:f:/t/01368-CPFScrumTeam/EqvejPq4h7JAm4w1KHgA6MUBf3UHLrvXkxnrMbNdqd8aGA?e=2lprxE

Done. Results can be found at
https://bcgov.sharepoint.com/:f:/t/01368-CPFScrumTeam/EqvejPq4h7JAm4w1KHgA6MUBf3UHLrvXkxnrMbNdqd8aGA?e=2lprxE


Create Tribute Funds page

Current page (for content): http://www.cancervaccine.org.uk/tributefunds.html

This page should live under Donate
Page should be based on collections, see this link for help

I've added this to donate page, it needs each file to be updated so the content displays correctly.

updated content. Done for now, will need some changes later.

Added text from existing site across. We still need to add in the images and possibly decide the layout


Please check your hook script, it should exit cleanly without doing anything on unknown/new hooks.

Any idea why this may be failing?
INFO: Using main config file /etc/dehydrated/config
./dehydrated: line 307: hook.sh: command not found
ERROR: Please check your hook script, it should exit cleanly without doing anything on unknown/new hooks.
./dehydrated: line 797: hook.sh: command not found
exit_hook returned with non-zero exit code!
root@letsencrypt:/etc/dehydrated#

I think it is fixed by https://github.com/SeattleDevs/letsencrypt-GoDaddy-hook/pull/5 now


Add configuration merging tool

Add a configuration merging tool to aid in migration between old-style
configurations and the new style configuration. Configurations can be
migrated by backing up tables using the config backup tool, then merging
them.
The merging is only necessary if configurations for multiple
configuration profiles are desired. This happens when multiple Pinot
clusters are managed using configuration profiles and a single source of
truth for configurations is desired.
For example, given multiple Pinot clusters in multiple data centers, one
might have two staging clusters and two production clusters. Since the
configuration backup tool only backs up the configuration for a single
Pinot cluster, without merging there would be one set of configurations
for each cluster. With config merging, one would end up with identical
keys merged across configurations (eg. table.name is the same across all
clusters) and different keys across configurations being merged (eg.
table.replication___staging=1 and table.replication___prod=3).
An optimization process is applied to minimize the number of generated
keys in the case where there are heterogenous combinations of clusters
with multiple profiles applied to them. For example, the combination
table.replication___staging=1 and table.replication___prod=3 is
preferred over table.replication___staging-us-west1=1,
table.replication___staging-us-east1=1,
table.replication___prod-us-west1=3, table.replication___us-east1=3 even
though both combinations expand to the same data.
Furthermore, a substitution process is applied to replace values by
configuration values read from the existing profiles. For example, if a
profile exists for "prod-us-east1" that contains
"kafka.broker=kafka-us-east1:2181", configuration keys that have the
value "kafka-us-east1:2181" and the "prod-us-east1" configuration
profile enabled will get replaced with "${kafka.broker}". In the case
where multiple replacements are possible, a minimization process is
applied to generate the smallest set of generated keys, similar to the
configuration optimization for keys that differ across clusters.
Thus, the migration process is:

Write the per profile configuration files (eg. profile/staging.conf,
profile/staging-us-east1.conf", ...)
Back up configuration files using the table config backup tool in
directories that contain the profile tags to be applied for that
cluster. For example, data from the staging-us-west1 cluster would be
backed up to configBackup/staging/staging-us-west1 so that
configuration files in that directory are understood to have the
staging and staging-us-west1 configuration profiles.
Invoke the configuration merger with the configBackup directory as
input.
Review the generated merged files for stylistic preferences, as
desired.

Codecov Report

Merging #3506 into master will increase coverage by 10.34%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             master    #3506       +/-   ##
=============================================
+ Coverage     60.47%   70.81%   +10.34%
  Complexity        4        4
=============================================
  Files           999      999
  Lines         45673    45692       +19
  Branches       6112     6117        +5
=============================================
+ Hits          27619    32356     +4737
+ Misses        16052    11217     -4835
- Partials       2002     2119      +117

Impacted Files
Coverage Δ
Complexity Δ

...lix/EmptyBrokerOnlineOfflineStateModelFactory.java
86.66% <0%> (-13.34%)
0% <0%> (ø)

.../pinot/core/segment/index/SegmentMetadataImpl.java
81.56% <0%> (+0.34%)
0% <0%> (ø)
:arrow_down:

...ot/core/segment/store/SegmentLocalFSDirectory.java
65.28% <0%> (+0.34%)
0% <0%> (ø)
:arrow_down:

...ator/transform/function/BaseTransformFunction.java
29.95% <0%> (+0.42%)
0% <0%> (ø)
:arrow_down:

...regation/function/customobject/QuantileDigest.java
58.31% <0%> (+0.44%)
0% <0%> (ø)
:arrow_down:

...linkedin/pinot/common/utils/helix/HelixHelper.java
54.16% <0%> (+0.59%)
0% <0%> (ø)
:arrow_down:

...java/com/linkedin/pinot/common/data/FieldSpec.java
75.6% <0%> (+0.6%)
0% <0%> (ø)
:arrow_down:

...ment/creator/impl/SegmentColumnarIndexCreator.java
88.41% <0%> (+0.77%)
0% <0%> (ø)
:arrow_down:

...r/transform/function/ValueInTransformFunction.java
39.2% <0%> (+0.8%)
0% <0%> (ø)
:arrow_down:

...com/linkedin/pinot/common/utils/ServiceStatus.java
70.73% <0%> (+0.81%)
0% <0%> (ø)
:arrow_down:

... and 310 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 402cf3b...ddccedb. Read the comment docs.

Will review it tomorrow
On Wed, Nov 21, 2018 at 1:29 PM Jean-François Im<EMAIL_ADDRESS>wrote:

@jfim https://github.com/jfim requested your review on: #3506
https://github.com/apache/incubator-pinot/pull/3506 Add configuration
merging tool.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
https://github.com/apache/incubator-pinot/pull/3506#event-1981137696,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAz7ZzuMRyhlIYNCnUAduWT0ppQJlxozks5uxcXGgaJpZM4Ym1Tb
.

Thanks. Can you add some test cases?

Closed this PR due to 6 months inactivity. Reopen if needed.


Send completionItem MarkupKind according to client support

Motivation

Sometimes, the client supports markdown, and we should able to send
markdown for completion item information in those cases.
Test plan

Tested locally in VS Code to see that the completion item was rendered as
markdown.
Thoughts on how to test this? Or did you say that you'd be diligent about
catching these sorts of cases in code review?

There's no easy way to test this. We test that hover uses Markdown in tests because we can make assertion on hover contents, so we check that e.g. sigs are put into code blocks.
We could write a protocol test (test/lsp/protocol_test_corpus).


feat: BlockchainDbMeta builder

Motivation
Ref #21

Solution
Adds builder methods to instantiate BlockchainDbMeta
 let block =
        provider.get_block(BlockId::latest(), BlockTransactionsKind::Hashes).await?.unwrap();

 let meta = BlockchainDbMeta::default()
        .with_chain_id(31337)
        .with_block(block.inner)
        .with_url(anvil.endpoint());

PR Checklist

[ ] Added Tests
[x] Added Documentation
[ ] Breaking changes

This is not breaking, we keep the BlockchainDbMeta::new() method as is.

idk what's up with the new map changes or how to fix @DaniPopes


fix all files in project

it would be cool to have the command "fix all files in project" (files that match the editorconfig rules)

+1

Closing as Atom is abandoned.


Proposal: text/template: add support for io.Reader

What version of Go are you using (go version)?

$ go version
go version go1.9.4 linux/amd64

Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (go env)?
go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/clap/go"
GORACE=""
GOROOT="/usr/lib/go-1.9"
GOTOOLDIR="/usr/lib/go-1.9/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build114129408=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?
	tpl := template.Must(template.New("").Parse("{{.}}"))
	tpl.Execute(os.Stdout, strings.NewReader("gopher"))

https://play.golang.org/p/QZvPZjmezPU
What did you expect to see?
"gopher"
What did you see instead?
"{gopher 0 -1}"
Rationale
Most IO operations in go are executed on io.ReadWriters and it is very frequent to open streams, pass them around, wrap them and so on, all the way from data sources to data sinks.
One exception to this rule seems to be templates: the only way to take some text blob from a file and output it in the template is to copy the entire file in a string and then pass the string to Execute.
It would be nice to be able to use io.Readers with templates and avoid the unnecessary buffering.
A CL for the proposed change can be found here.
A previous bug mentioning this need: #25160.

Sounds reasonable, given that there's no simple way to currently work around buffering all the data in memory.
I wonder how often this comes up, though. I've used templates for multiple years and I've never had the need to print an io.Reader with them. Do you have example use cases?
/cc @robpike @rogpeppe

Sadly, none that I can link, but I've encountered this problem in several situations, mostly because I dealt a lot with net conns and external data sources that my code was just transforming (wrapping readers).
I ended up writing my own kind of templates that interleaved (*text.Template).Execute calls with io.Copy calls.
I just found the previous bug and created a small patch to support this use case for me and other people that might incur in the same issue.
Also, since most of the stdlib passes io.Readers around, it just seemed more natural and fitting to print the content of the reader instead of the reflection of its fields values, which seems like a very edge case for templates.

I've re-purposed this to be a proposal, in hopes that the proposal team can make a decision.
I still think this is reasonable, though I haven't encountered the use case directly. It doesn't add any new API, and shouldn't break any existing templates. So I lean towards doing this, since none of the other template owners have spoken up.

/cc @robpike

Trying to do this in the template language seems quite complex. Maybe {{.}} makes sense to copy directly, but what about ``{{. | printf "%q"}}`? That would still have to buffer. There are a lot of API implications here, so it really is "new API".
Getting back to:
tpl := template.Must(template.New("").Parse("{{.}}"))
tpl.Execute(os.Stdout, strings.NewReader("gopher"))

If the template does no buffering of its own, or if we gave a way to flush the buffer, then you could define your own "copy" function that copies a reader to stdout and write {{copy .}} instead. Would that work?

Probably defining a copy function would solve the problem, but how would you pass the current io.Writer the template is rendering to?

I talked to @robpike (owner of text/template) about this a while back, and we weren't entirely sure about the implications. I'm still not. The current CL handles an io.Reader coming out of the end of execute, but what would that mean for html/template? Does html/template have to have streaming escaping converters? Should anything else change?
Does anyone want to write down the specific documentation changes that would describe the proposed change? Note that CL 25160 exempts Readers that are also Stringers or errors from being read, and I don't quite understand why.
Existing templates may also be affected unexpectedly. Perhaps a design doc is warranted here.

I ended up doing something like this, basically passing the output writer to the template and wrapping io.Copy in a helper.
The more I think about embedding this behavior in the templating language the less I'm convinced about it.
Closing as this can be done with current templates and adding helpers to simplify it would significantly complicate things.
Thanks for working on this.


man: Fix HTML formatting for ++ operator

+ in asciidoc has special meaning. Sometimes this causes formatting corruption. Avoid by using dedicated escapes.

Checklist

[ ] Language changes are updated in man/adoc/bpftrace.adoc
[ ] User-visible and non-trivial changes updated in CHANGELOG.md
[ ] The new behaviour is covered by tests

Sneaking this in before our next in-person lab tomorrow. Please yell at me if I got it wrong :P

Yay! Thanks for fixing @danobi. This was driving me crazy.


Crash on release build

i tried to build my app on release mode, and when i opening the broadcast page it's crashed

I don't know what error u're talking about? u can post an error code up here?


Incorrect file permissions match

On nearly every ADC we've got this match:
files with permissions 644:
///netscaler/portal/templates/wrapper.tmpl
///netscaler/portal/templates/bookmark.html
///netscaler/portal/templates/changepwd.html
///netscaler/portal/templates/chpwdsuccess.html
///netscaler/portal/templates/err2002.html
///netscaler/portal/templates/err2004.html
///netscaler/portal/templates/err2005.html
///netscaler/portal/templates/err2006.html
///netscaler/portal/templates/error.html
///netscaler/portal/templates/f_services.html
///netscaler/portal/templates/filetransfer.html
///netscaler/portal/templates/ftlist.html
///netscaler/portal/templates/globalBK.tmpl
///netscaler/portal/templates/globalFS.tmpl
///netscaler/portal/templates/homepage.html
///netscaler/portal/templates/homepage2.html
///netscaler/portal/templates/j_services.html
///netscaler/portal/templates/loadresources.tmpl
///netscaler/portal/templates/menu.tmpl
///netscaler/portal/templates/missing.html
///netscaler/portal/templates/navwrapper.tmpl
///netscaler/portal/templates/newbm.html
///netscaler/portal/templates/ping.html
///netscaler/portal/templates/preferences.html
///netscaler/portal/templates/resources.tmpl
///netscaler/portal/templates/rmbm.html
///netscaler/portal/templates/rmft.html
///netscaler/portal/templates/styles.css
///netscaler/portal/templates/tips.html
On a fresh installed ADC 13 there are the same files with the same permissions.
Is there any chance to exclude these files?
Because of this the summary shows "Evidence of compromise found: yes" but it isn't.

when we reviewed compromised netscalars, files created by the vulnerable processes typically had permissions 644 which was inconsistent with the permissions that we found associated with files created during a "normal" installation. so, this match is a fairly low confidence signal that something might be wrong. however, if you have other context that explains the match (its a pristine install, Citrix updated their installer, etc.) then i think its probably ok to ignore.

I have downloaded the latest ADC 12.1 tgz file and as I can see these permissions are default:

We've opended a Citrix case and got this answer:
**There is a known issue due to which the security team is already working on this
===>Engineering(Security team) has opened a bug ID  - CGOP-14525, under the title- 'Permission model for files in /netscaler/portal/templates regressed'
===>The security team has confirmed that this is a regression and they are aware of this issue. They are working with engineering to reset the appropriate permissions.
===>There's no vulnerability noted at this point, as it pertains to only file permissions
===>The ETA for patching is at present not confirmed, the issue is being actively tracked by the team.
So you can safely ignore the same.**


Issues with password change

Password change occurs on a completely unrelated domain. The successful action leaves me on that domain, and a confused user.
Because of above - it breaks the way password managers work - I let my password manager generate a random password, and it does so on the new domain, and therefore does not associate my password with the actual domain I want to log in to (nor with an email or username, as that is not part of the reset password form). As a heavy random password user, this never happens.

Documenting the old process:

The redirection will go to auth0 side as auth0 manages the password change.

right now the process is
graph TD
A("Don't remember your password?") --> B
B("Enter email ") --> C
C("Auth0 send email with reset password link like this") --atomatichq.eu.auth0.com/lo/reset?ticket=*--> D
D("Enter new password") --Success--> E
E("Login in 'datapackaged.com'") --> F("Success")

INVALID. We've switched off Auth0 and won't have password login - see #268


SIL: Don't skip lowering obsolete decls

When -unavailable-decl-optimization=complete is specified obsolete decls should be preserved because their symbols are still ABI since they are available to use when targeting deployment targets earlier than the obsoletion version.
Resolves rdar://110268649

@swift-ci please test


Distribute bundled lib as UMD module

This makes our npm package a bit more browser-friendly by bundling the library as a UMD module. The npm prepublish script gets automatically run when we publish to npm so that we don't have to check-in the bundled files manually (nor remember to update them).
@dmitrizagidulin @deiu

Nicely done. 👍

💃


Can't open the email link

Subject: Can't open the email link
Description:
Opened the link
Results:
It somehow restricted
Backend version: develop-7c33e18d
Frontend version:
User Agent: Mozilla/5.0 (Linux; U; Android 10; ru-ru; Mi 9T Build/QKQ1.190825.002) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Chrome/71.0.3578.141 Mobile Safari/537.36 XiaoMi/MiuiBrowser/12.4.4-g
Page: https://couchers.org/restricted
User: garry (2081)

not enough info or can't reproduce


Update Chronos version

This release includes a number of fixes including the fix for the web proxy.

please take a look ASAP @jsancio or @ConnorDoyle

cc @mgummelt or anyone else who would like to review

:shipit:


Robot sometimes overshoots into the defence area

We might want to increase our defense area margin such that robots avoid it better

this is harder than it seems,
you need to allow to be at the edge of the defense area, while at the same time make sure that the robots do not overshoot at all into them.


[swiftc (79 vs. 5179)] Add crasher in ?

Add test case for crash triggered in ?.
Current number of unresolved compiler crashers: 79 (5179 resolved)
Assertion failure in lib/AST/Type.cpp (line 1280):
Assertion `Result && "Case not implemented!"' failed.

When executing: swift::CanType swift::TypeBase::getCanonicalType()

Assertion context:
  }
  }

  // Cache the canonical type for future queries.
  assert(Result && "Case not implemented!");
  CanonicalType = Result;
  return CanType(Result);
}

Stack trace:
swift: /path/to/swift/lib/AST/Type.cpp:1280: swift::CanType swift::TypeBase::getCanonicalType(): Assertion `Result && "Case not implemented!"' failed.
Stack dump:
0.	Program arguments: /path/to/swift/bin/swift -frontend -c -primary-file validation-test/compiler_crashers/28447-result-case-not-implemented-failed.swift -target x86_64-unknown-linux-gnu -disable-objc-interop -module-name main -o /tmp/28447-result-case-not-implemented-failed-778155.o
<unknown>:0: error: unable to execute command: Aborted
<unknown>:0: error: compile command failed due to signal (use -v to see invocation)

@swift-ci please smoke test and merge

@swift-ci please test and merge

I am disabling this in https://github.com/apple/swift/pull/5612 because it seems to not be 100% deterministically crashing.  I filed SR-3118 so that we don't lose track of the crash.


Update virtualenv along with setuptools.

Using older virtualenv with setuptools above version 60 can lead to undesirable behavior. Example of these problems are:

Unexpected layout within the virtual environment
Failure to isolate virtual environment from system-wide packages

Fixes #225

@mkalcok please could you add some testing evidence that shows the the change does indeed fix #225; just a paste of a successful build, or a link to a review which has this version of layer-basic vendored in would be good.  It's just that layer-basic affects so many charms!

Yup, I understand. I'll try to trigger a CI run for ovn-chassis (which is where the problem occurred initially) with this change vendored in


Add inspection module to replace print_routes

Summary of Changes
Create an inspection module to replace the print_route script.
This was inspired by the discussion about #1435 but has evolved in a inspection module that can obtain information regarding the application and create a pretty print of it.
The public interface of the inspection module has different functions that return information regarding an application.
There is a main function to inspect the entire application (routes, middleware, static_routes, sinks and error_handlers) and also individual functions for each.
The returned information are instance of classes (or list of them) which can return a pretty print, since this is more extensible than just a string.
There is also the ability to register inspection of different route class implementations by using a decorator on a inspection function that handles a particular type of router.
This is the output of a sample application
Falcon App (WSGI)
• Routes:
    ⇒ /foo - MyResponder:
       ├── DELETE - on_delete
       ├── GET - on_get
       └── POST - on_post
    ⇒ /foo/{id} - MyResponder:
       ├── DELETE - on_delete_id
       ├── GET - on_get_id
       └── POST - on_post_id
    ⇒ /bar - OtherResponder:
       ├── DELETE - on_delete_id
       ├── GET - on_get_id
       └── POST - on_post_id
• Middleware (Middleware are independent):
    → MyMiddleware.process_request

      ↣ MyMiddleware.process_resource
        ↣ OtherMiddleware.process_resource

            ├── Process responder

        ↢ OtherMiddleware.process_response
      ↢ CORSMiddleware.process_response
• Static routes:
    ↦ /tests/ C:\Users\Federico\Dev\GitHub\falcon\tests [C:\Users\Federico\Dev\GitHub\falcon\tests\conftest.py]
    ↦ /falcon/ C:\Users\Federico\Dev\GitHub\falcon\falcon
• Sinks:
    ⇥ /sink_cls SinkClass
    ⇥ /sink_fn sinkFn
• Error handlers:
    ⇜ RuntimeError my_runtime_handler

A more verbose output is

View
Falcon App (WSGI)
• Routes:
    ⇒ /foo - MyResponder (c:\users\federico\dev\github\falcon\try_print.py:19):
       ├── DELETE - on_delete (c:\users\federico\dev\github\falcon\try_print.py:26)
       ├── GET - on_get (c:\users\federico\dev\github\falcon\try_print.py:20)
       ├── POST - on_post (c:\users\federico\dev\github\falcon\try_print.py:23)
       ├── OPTIONS - options_responder (c:\users\federico\dev\github\falcon\falcon\responders.py:84)
       ├── CONNECT - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── HEAD - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── PATCH - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── PUT - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── TRACE - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── CHECKIN - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── CHECKOUT - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── COPY - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── LOCK - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── MKCOL - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── MOVE - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── PROPFIND - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── PROPPATCH - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── REPORT - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── UNCHECKIN - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── UNLOCK - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── UPDATE - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       └── VERSION-CONTROL - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
    ⇒ /foo/{id} - MyResponder (c:\users\federico\dev\github\falcon\try_print.py:19):
       ├── DELETE - on_delete_id (c:\users\federico\dev\github\falcon\try_print.py:35)
       ├── GET - on_get_id (c:\users\federico\dev\github\falcon\try_print.py:29)
       ├── POST - on_post_id (c:\users\federico\dev\github\falcon\try_print.py:32)
       ├── OPTIONS - options_responder (c:\users\federico\dev\github\falcon\falcon\responders.py:84)
       ├── CONNECT - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── HEAD - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── PATCH - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── PUT - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── TRACE - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── CHECKIN - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── CHECKOUT - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── COPY - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── LOCK - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── MKCOL - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── MOVE - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── PROPFIND - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── PROPPATCH - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── REPORT - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── UNCHECKIN - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── UNLOCK - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── UPDATE - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       └── VERSION-CONTROL - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
    ⇒ /bar - OtherResponder (c:\users\federico\dev\github\falcon\try_print.py:39):
       ├── DELETE - on_delete_id (c:\users\federico\dev\github\falcon\try_print.py:35)
       ├── GET - on_get_id (c:\users\federico\dev\github\falcon\try_print.py:29)
       ├── POST - on_post_id (c:\users\federico\dev\github\falcon\try_print.py:32)
       ├── OPTIONS - options_responder (c:\users\federico\dev\github\falcon\falcon\responders.py:84)
       ├── CONNECT - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── HEAD - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── PATCH - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── PUT - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── TRACE - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── CHECKIN - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── CHECKOUT - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── COPY - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── LOCK - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── MKCOL - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── MOVE - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── PROPFIND - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── PROPPATCH - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── REPORT - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── UNCHECKIN - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── UNLOCK - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       ├── UPDATE - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
       └── VERSION-CONTROL - method_not_allowed (c:\users\federico\dev\github\falcon\falcon\responders.py:59)
• Middleware (Middleware are independent):
    → MyMiddleware.process_request

      ↣ MyMiddleware.process_resource
        ↣ OtherMiddleware.process_resource

            ├── Process responder

        ↢ OtherMiddleware.process_response
      ↢ CORSMiddleware.process_response
    - Middlewares classes:
        ↣ CORSMiddleware (c:\users\federico\dev\github\falcon\falcon\middlewares.py:2):
           └── process_response (c:\users\federico\dev\github\falcon\falcon\middlewares.py:3)
        ↣ MyMiddleware (c:\users\federico\dev\github\falcon\try_print.py:73):
           ├── process_request (c:\users\federico\dev\github\falcon\try_print.py:74)
           └── process_resource (c:\users\federico\dev\github\falcon\try_print.py:77)
        ↣ OtherMiddleware (c:\users\federico\dev\github\falcon\try_print.py:83):
           ├── process_resource (c:\users\federico\dev\github\falcon\try_print.py:87)
           └── process_response (c:\users\federico\dev\github\falcon\try_print.py:90)
• Static routes:
    ↦ /tests/ C:\Users\Federico\Dev\GitHub\falcon\tests [C:\Users\Federico\Dev\GitHub\falcon\tests\conftest.py]
    ↦ /falcon/ C:\Users\Federico\Dev\GitHub\falcon\falcon
• Sinks:
    ⇥ /sink_cls SinkClass (c:\users\federico\dev\github\falcon\try_print.py:57)
    ⇥ /sink_fn sinkFn (c:\users\federico\dev\github\falcon\try_print.py:53)
• Error handlers:
    ⇜ Exception _python_error_handler (c:\users\federico\dev\github\falcon\falcon\app.py:894)
    ⇜ HTTPError _http_error_handler (c:\users\federico\dev\github\falcon\falcon\app.py:891)
    ⇜ HTTPStatus _http_status_handler (c:\users\federico\dev\github\falcon\falcon\app.py:888)
    ⇜ RuntimeError my_runtime_handler (c:\users\federico\dev\github\falcon\try_print.py:66)

This is still very much still a proof of concept wip, with documentation and test missing. I wanted to open this PR to get feedback on this.
The main items that are missing are:

verify that everything work with an async app
tests
documentation on it. I think it would be useful if we include the output of the inspection in the example documentation to better visualize the configuration of the example

Related Issues
This fixes #1435
Pull Request Checklist
This is just a reminder about the most common mistakes.  Please make sure that you tick all appropriate boxes.  But please read our contribution guide at least once; it will save you a few review cycles!
If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing to do.

[x] Added tests for changed code.
[x] Prefixed code comments with GitHub nick and an appropriate prefix.
[x] Coding style is consistent with the rest of the framework.
[x] Updated documentation for changed code.

[x] Added docstrings for any new classes, functions, or modules.
[x] Updated docstrings for any modifications to existing code.
[ ] Added references to new classes, functions, or modules to the relevant RST file under docs/.
[ ] Updated all relevant supporting documentation files under docs/.
[x] A copyright notice is included at the top of any new modules (using your own name or the name of your organization).
[x] Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives.

[ ] Changes (and possible deprecations) have towncrier news fragments under docs/_newsfragments/. (Run towncrier --draft to ensure it renders correctly.)

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!
PR template inspired by the attrs project.

The code and the tests should be done.
The documentation is still missing. I was thinking of adding a new documentation page with a brief explanation of the module and an example output similar to the one in this page.
Also maybe mention it in some tutorial examples, like where it's adding the roues and the one on the middleware?

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@98929d5). Click here to learn what that means.
The diff coverage is 100%.

@@           Coverage Diff            @@
##             master   #1661   +/-   ##
========================================
  Coverage          ?    100%
========================================
  Files             ?      49
  Lines             ?    3895
  Branches          ?     606
========================================
  Hits              ?    3895
  Misses            ?       0
  Partials          ?       0

Impacted Files
Coverage Δ

falcon/status_codes.py
100% <100%> (ø)

falcon/inspect.py
100% <100%> (ø)

falcon/asgi/structures.py
100% <100%> (ø)

falcon/media/multipart.py
100% <100%> (ø)

falcon/routing/compiled.py
100% <100%> (ø)

falcon/asgi/request.py
100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98929d5...6d630e3. Read the comment docs.

I've added a new documentation file for the inspect module and updated the quickstart and tutorial files to introduce the inspect module.
I think this is ready for a review

Take your time, it ended up beeing quite large :+1:
The suffixed responders are not treated differently than the normal ones, but the name of the function is correctly printed, like "on_get_with_suffix", but maybe there should be an attribute in the route method class that contains the suffix?
Also I was wondering if we should separate the internal routers from the source info print. Now both are under the verbose flag. Maybe we could add an "internal" flag?

The suffixed responders are not treated differently than the normal ones, but the name of the function is correctly printed, like "on_get_with_suffix", but maybe there should be an attribute in the route method class that contains the suffix?

TBH I think it looks good as-is.

Also I was wondering if we should separate the internal routers from the source info print. Now both are under the verbose flag. Maybe we could add an "internal" flag?

That would be great, actually.

@kgriffs I've split the internal and the verbose options.

Hope you don't mind my tweaks... just trying to polish up the prose a bit.

Hope you don't mind my tweaks... just trying to polish up the prose a bit.

No, not at all. I'm not very good with the documentation :(

Nice!


Can't install on PHP 7.3

I'm trying to run command:
composer require workerman/workerman
But get an error:
You are running Composer with SSL/TLS protection disabled.
Using version ^3.5 for workerman/workerman
./composer.json has been updated
Loading composer repositories with package information
Updating dependencies (including require-dev)

Installation failed, reverting ./composer.json to its original content.

  [ErrorException]
  "continue" targeting switch is equivalent to "break". Did you mean to use "continue 2"?

Any ideas how to install it on PHP 7.3?

Just need to update Composer. Sorry )


macOS version does not show up in the Dock, app switcher, or menu bar

When double-clicking the Mark Mind icon on macOS, the application window shows up, but there is no Dock icon, and no tab-switcher icon, making it impossible to return to the app except by double-clicking it again. Even a menu bar icon would be useful.

This API might help: https://www.electronjs.org/docs/latest/tutorial/macos-dock/
Happy to assist with the testing, if required.

you can use this software  https://github.com/MarkMindCkm/MarkMind-ltd


New standalone fork

Hello.
I made an independent fork for my personal needs: https://github.com/ange007/paroller.js-modern
Since the code is heavily rewritten then the usual PR seems to me not appropriate.
If the work done is of interest, you can transfer the code here.
Changes from the original:

AMD and CommonJS init support
New plugin structure (class/prototype)
Recalculation when resizing a window
reinit and destroy functions
Removed code duplication
Other...

@ange007 is there a usage reference?
It doesn't seem to work in the same way.

@toddpadwick
At the moment, no, because it was used only for themselves.

so instead of being able to help ghost with his problem you just made it worse


973 sendsync

Overview
The Broadcaster interface function SendSync is now handled through a new /cluster/message endpoint. This allows us to gather and return errors that any node in the cluster could generate through any of the broadcasted messages like: CreateIndexMessage
Additional, Broadcaster.SendAsync is passed through to a different SendAsync function which is part of the Gossiper interface.
This PR contains the work started by @raskle in #984, but I adjusted it a bit so that the BroadcastHander interface didn't require a SendSync function.
Fixes #973
Pull request checklist

[x] I have read the contributing guide.
[x] I have agreed to the Contributor License Agreement.
[x] I have updated the documentation.
[x] I have resolved any merge conflicts.
[x] I have included tests that cover my changes.
[x] All new and existing tests pass.

Code review checklist
This is the checklist that the reviewer will follow while reviewing your pull request. You do not need to do anything with this checklist, but be aware of what the reviewer will be looking for.

[ ] Ensure that any changes to external docs have been included in this pull request.
[ ] If the changes require that minor/major versions need to be updated, tag the PR appropriately.
[ ] Ensure the new code is properly commented and follows Idiomatic Go.
[ ] Check that tests have been written and that they cover the new functionality.
[ ] Run tests and ensure they pass.
[ ] Build and run the code, performing any applicable integration testing.

Thanks @jaffee, I improved those error messages in client.SendMessage().


docs: docker compose config for running nsqd, nsqlookupd, and nsqadmin

Here is a fig.yml that can run the entire nsq stack easily for demo and evaluation.
Make sure to plugin your real
nsqlookupd:
  image: nsqio/nsqlookupd
  ports:
    - "4160:4160"
    - "4161:4161"
  command: -broadcast-address=<host>

nsqd:
  image: nsqio/nsqd
  ports:
    - "4150:4150"
    - "4151:4151"
  command: -broadcast-address=<host>  -lookupd-tcp-address=<host>:4160

nsqadmin:
  image: nsqio/nsqadmin
  ports:
    - "4171:4171"
  command: -lookupd-http-address=<host>:4161

@mingfang do you want to update this for the new single docker image and add this to our docker docs (in the gh-pages branch)

updated title to reflect fig being replaced by compose

See nsqio/nsqio.github.com#11


Inline links could be more visible in the documentation

Maybe my eyesight is not good, but I think the inline links in the docs could be a little more distinguishable:

when pasted here it actually looks better than on my screen because the dark font of main text is a little brighter in the example above (interpolation, I think).

Good old underline maybe?

Is it the same as https://pydata-sphinx-theme.readthedocs.io/en/latest/ ? If so it would be good to open an issue there first, and address here with CSS overrides only if they reject the idea of making the links clearer somehow

@larsonerYes, it looks the same. Ok, I'll try that next week!

They have adjusted this so I'll close. If it's still a problem let's work upstream!


::execute(), ::setFactory() and ::set()

Apologies for my recent absence - I've had an obscene amount of work over the last few weeks and I'm not even close to the end. Regardless, I've had some concerns that can't be delayed any longer as (happily) we're progressing on with this.
My main issue is a personal U-turn in opinion regarding using ::execute() as a way to give scope to the static proxy methods. The code below is an attempt to illustrate the problem - which is that no service can safely and easily perform watcher operations. This is because there is no guarantee that the currently active loop driver is the one that issued the watcher. In the example below, the two ping calls could easily occur in different loops and cause a problem.
class Client
{
    private $stream;
    private $watcher = null;

    public function __construct($stream)
    {
        $this->stream = $stream;
    }

    public function ping()
    {
        if ($this->watcher !== null) {
            // This line could screw up
            Loop::cancel($this->watcher);
        }

        $this->watcher = Loop::onWritable($stream, function () use ($stream) {
            fwrite($stream, 'ping');
        });
    }
}

Loop::execute(function () {
    $client = new Client(...);

    $httpClient->get('/some-resource')->success(function () use ($client) {
        $client->ping();
    });

    // some stuff

    $client->ping();
});

The Client could store the watchers in the loop registry, but this is going to make the code very ugly, very quickly. Given that the purpose of this design is to avoid dependency injection with the hope of simplifying, I'd argue that we should re-evaluate if the current approach actually helps to remove complexity at all.
Although I could be persuaded, currently I think I would prefer ditching the registry and using ::set(), ::get(), ::clear() (or whatever) and making it so that consumers must manage the state of the loop themselves.
This leads on to my second issue, which is that we sort of accidently already have ::set() because calling ::setFactory() then any of the proxy methods will automatically create a loop and activate it - despite not being within the bounds of an :execute() call. I'm not really sure I have a strong opinion on this - it just feels like there's no clear way to use the API that we currently have.

I don't think I understand your problem. I honestly think you're seeing a highly theoretical problem where there's no practical one.
Loop::execute() is really just like a manual $old = get(), set($new), $callback(), set($old) sequence.
It's basically what you need to do anyway there.
You have exactly the same problem when managing set/get yourself, just that it's probably more error-prone.
You never should move objects containing watchers into a separate loop scope than they were defined in. get/set/clear won't make any difference here. I've never seen this actually being a problem.
In fact, it helps a lot. I've had a lot of problems with the manual setting in past, especially when writing tests. Like, tests don't clean up perfectly, then I need to swap the loops; but it didn't happen on this test and debugging that down it painful. ::execute() nicely solves this by just installing a new loop every time.
setFactory() can only be called at the top-level anyway … set() is quite specific and could be called anytime.

I've never seen this actually being a problem.

That's not true. We've had that exact issue with amphp/dns. But it's an issue we can't really prevent.

You never should move objects containing watchers into a separate loop scope than they were defined in. get/set/clear won't make any difference here. I've never seen this actually being a problem.

I don't think that's something you can control. If you're creating a service you can't control where in the call stack your code is called from. Given the layers of callbacks that occur in this type of programming, I can see this being a frequent problem.

Also, it doesn't activate the loop; there's just the hack with Loop::execute($cb, Loop::get()), which is used to emulate synchronous code ("wait"). In normal code you're all the time inside at least one level of Loop::execute() and thus that's a non-issue.

I don't think I disagree with you, and I get that it can only be done at the top level - but I'm not comfortable with this sort of thing being valid with the current API design (as I believe it currently is):
Loop::setFactory(new LoopFactory);

Loop::onReadable(...);
Loop::onReadable(...);
// ...

Loop::get()->run();

What do we gain by allowing the loop to be used outside of ::execute()?

Given the layers of callbacks that occur in this type of programming, I can see this being a frequent problem.

Multiple event loops will be very, very rare, so I don't think this will be a frequent problem.

What do we gain by allowing the loop to be used outside of ::execute()?

The ability to support synchronous wait for applications not entirely written within an event loop.

If multiple event loops will be very, very rare - and the API we have here will run into the issues highlighted when they are used - I think we should do more to make it clear that the consumers are on their own and not supported by the standard when they go down this path.
It feels wrong supporting a very, very rare use case that we know has the potential to encourage bugs when used (unless everyone adopts very defensive programming).
If a consumer wants multiple loops, there's nothing stopping them using the drivers directly and I feel this would be much safer - as then a consumer/service/package could guarantee that the return value of Loop::onReadable could always be passed to Loop::disable, for example (which they current can't).
We could implement the same sort of protection against accidental event loop overwriting with ::set(), ::get() - just by making it so that you couldn't set an event loop on top of an existing one. If there really is a need for it, we could offer ::replace(), with warnings of the dangers.

I think we should do more to make it clear that the consumers are on their own and not supported by the standard when they go down this path.

Not supporting a clean shutdown in Aerys sounds like a bad idea. Maybe @bwoebi can shed more light on why we need a new loop there.

If a consumer wants multiple loops, there's nothing stopping them using the drivers directly and I feel this would be much safer

I can't agree here. Every single package will coded using the global accessor, you won't be able to inject your custom loop anywhere then, thus use none of your usual APIs.

return value of Loop::onReadable could always be passed to Loop::disable

This is guaranteed as long as you don't use objects across multiple loops.

just by making it so that you couldn't set an event loop on top of an existing one

That's exactly a need for clean shutdowns.

@kelunik We have that issue with amp/dns because it uses static. But that's exactly the reason why we have fetchState/storeState.
Now it looks like this: https://github.com/amphp/file/blob/amp_v2/lib/functions.php#L14-L25 and won't make any problems [this is for amp/file, but will look similar in amp/dns].
With the loop as we have designed it currently, all the places where it was an issue until now, aren't an issue anymore.
@AndrewCarterUK

What do we gain by allowing the loop to be used outside of ::execute()?

as it was said, "wait" support.  Just because something is possible and can be used for a few well-selected use cases, you shouldn't necessarily do it. Loop::get() really should only be used by few libraries; the main reasons to use it currently are the wait function and to get the underlying handle.

It feels wrong supporting a very, very rare use case that we know has the potential to encourage bugs when used (unless everyone adopts very defensive programming).

While rare, aerys uses it twice [apart from the initial main loop start]. Once to try to continue a loop in a shutdown handler (an attempt of proper cleanup) [Good, that's not actually nested, it's just Loop::execute(..., Loop::get());].
The other time it creates a new execution context in the stopping function to send a stop signal to the master process in a shutdown handler, which must not be interrupted by anything else. [To be sure that there won't be randomly induced fatals from otherwhere killing aerys before having notified the master.]
So basically, the main use case is uninterruptible (from within the process) execution of something. If you begin deeply nesting your execute()'s, you clearly have misunderstood something. In 99% of time you should only be in the top most loop level. Only use nested Loop::execute() if you really need it and with care.

@kelunik
What you're saying suggests that using multiple event loops is not a rare case? I don't think this can go both ways. Either they're common, and we have a problem because the target of the static problem is switching - or they're rare - in which case, why are we allowing the safety API to be compromised by a non-standard use case?

return value of Loop::onReadable could always be passed to Loop::disable

This is guaranteed as long as you don't use objects across multiple loops.

Which can't be guaranteed. If you're writing a service or a package, you have no control over where your service is called from. You couldn't even do a clean up in a destructor because you have no idea where in the call stack that might happen.

I can't agree here. Every single package will coded using the global accessor, you won't be able to inject your custom loop anywhere then, thus use none of your usual APIs.

Then I would request that we consider removing the static proxy and advocating dependency injection instead.

To best summarise my opinion: switching the target for the static proxy functions is a dangerous operation that encourages bugs and confusion. If I was creating say a HTTP client using this standard, I would be very concerned that I could not guarantee I was accessing the same loop (or any loop at all) for the public methods in my class.
The current design of the static proxy requires you to use the registry to store watchers if you want to operate safely. I can't support this, and I think it introduces more complexity than dependency injection (which is a well used and understood practice) would.
I don't have the experience building async apps that others have, but I also find it a bit confusing that Javascript applications can function with one native event loop but the same cannot be achieved in PHP. That's probably more a gap in my understanding - but I feel lit is something we should strive for if possible.
Apologies for my complete U-turn on this topic, but I'm not convinced we're heading down the right road.

They are rare, but necessary.
And my point still stands; execute() is safer than manual set().

Which can't be guaranteed. If you're writing a service or a package, you have no control over where your service is called from. You couldn't even do a clean up in a destructor because you have no idea where in the call stack that might happen.

Good point; Loop::execute() perhaps should call gc_collect_cycles() to prevent the destructors leaking into another loop.
And yes, you don't have control nor is it your responsibility as a library. It's the users responsibility to not move async objects (i.e. objects which control watchers somehow) into a separate loop. You can compare that to serializing objects and passing them to another process. The other process could unserialize it; but things will break the exactly same way now. And different loop scopes work just exactly like that.

Then I would request that we consider removing the static proxy and advocating dependency injection instead.

Dependency injection won't help it, because the common issue here is then that you yield some Promise which will never be resolved as the loop currently in task is an other one than where the watchers were installed.

The current design of the static proxy requires you to use the registry to store watchers if you want to operate safely. I can't support this, and I think it introduces more complexity than dependency injection (which is a well used and understood practice) would.

No. Don't do that. The registry is meant for global features which are bound to a loop; e.g. amp/dns … you only ever have a single active DNS resolver. There's no point in having two different resolvers active at the same point. … or amp/file … you only have one active file driver [having two file drivers active simultaneously could even break things if I remember correctly.] …
For everything else, that's a very bad idea.
Sure, you could reach a little higher degree of safety, but this will come at a cost (complexity in code to check all the time at the library boundaries whether we have switched loops… that's insane).
And as said, dependency injection will just shift the problems, but they are still there and there's nothing you can do about it.

In general, it's not like your points are totally invalid, but the alternatives really are worse, in more hidden and hideous ways.
Regarding Javascript, it is an issue there. They recently expanded a bit on their *Sync API to mitigate the problem, but the separate loop is largely an implementation issue for how node.js core is structured.
The current way is really "rewrite the asnyc code in a sync way" … hacky and messy. Not reusable etc..
 We (I and Niklas) have written a lot of async code and seen where we lost our time with debugging and API problems. … We once had the dependency injection. We did away with it and were happy with the results. Some new problems surfaced; which were often just old problems in another form. The static API with execute() and fetch/storeState(), is really a result of that. It solves the problems we are still observing with the current Amp version. It's not perfect, but it's far, far better than pure dependency injection was. It vastly reduced the ways you can shoot yourself into your own feet.

@kelunik / @bwoebi - thanks for your replies. I'll take some time to read through again and when I get a moment I'll check some of the AMP projects.

As an off the cuff thought - is there any way to make

@AndrewCarterUK Any way to make? Wanted to send something which got cut off?

Sorry - was going to send something which I've decided against - didn't realised I'd sent the half message - apologies for the notifications.

@AndrewCarterUK And further thoughts? If not, may you please close the issue?

Hi all -
I've had no time to resolve my thoughts and I don't believe I will in the near future. It would be best if everyone considered my involvement with async-interop suspended - as I don't wish to hold up development for those waiting for opinions from the group.
Thanks to everyone for all the work and good luck :)

@AndrewCarterUK Oh :-( If you're able to find any time nevertheless, you're more than welcome though ;-)


Use multiple graphs in mini batch generator

Hi,
Is there a way to use the ClusterNodeGenerator on a list of graphs?
Thanks,
-David

Hi @davidxujiayang, there isn't at the moment. A work-around for now would be to concatenate each subgraphs into one huge graph. That is, instead of having many StellarGraph objects, have one. I guess one risk is that training may be inefficient, because there will be a lot clusters that have nodes from many graphs, and not many edges will be being considered.
Could you say a bit about what sort of graphs you're working with and the problem you're solving? Maybe there's an alternative approach we can use easily.

Hi Huon,
Thanks for the reply! My current work around is I actually modified the
generator a bit so that the output shape for the output feature is (n_case,
n_nodeid, n_feature), where n_case is the number of training graphs,
instead of the current (1, n_nodeid, n_feature). One drawback I realize is
that this way is that in each batch the cluster selected for all cases are
the same. Do you think there are other problems with it?
The problem I'm trying to address is to predict pressure distribution on
different CFD meshes. So the input features are x,y coordinates as well as
a distance function that defines the distance between each point and the
wall, the target I want to predict is the pressure (a scalar value on each
point). I'm still prototyping with the clustered GCN, if you have any
suggestions on this it would be very appreciated!
Thanks,

David

Xu, Jiayang (David)
PhD Candidate
Computational Aerosciences Laboratory
Department of Aerospace Engineering
University of Michigan
On Sun, Apr 19, 2020 at 6:57 PM Huon Wilson<EMAIL_ADDRESS>wrote:

Hi @davidxujiayang https://github.com/davidxujiayang, there isn't at
the moment. A work-around for now would be to concatenate each subgraphs
into one huge graph. That is, instead of having many StellarGraph
objects, have one. I guess one risk is that training may be inefficient,
because there will be a lot clusters that have nodes from many graphs, and
not many edges will be being considered.
Could you say a bit about what sort of graphs you're working with and the
problem you're solving? Maybe there's an alternative approach we can use
easily.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stellargraph/stellargraph/issues/1275#issuecomment-616239470,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AFZJFPSIQ5FNWVS6GQSSPGLRNN6T3ANCNFSM4ML5GEEQ
.

From your description, that sounds like a very reasonable work-around. If you're comfortable sharing the code, we might even be able to put it into the library.

One drawback I realize is that this way is that in each batch the cluster selected for all cases are the same. Do you think there are other problems with it?

I'm not sure how well it generalises to graphs with different sizes, where the clusters won't all have the same number of elements. Are your graphs all the same size?

The problem I'm trying to address is to predict pressure distribution on
different CFD meshes. So the input features are x,y coordinates as well as
a distance function that defines the distance between each point and the
wall, the target I want to predict is the pressure (a scalar value on each
point). I'm still prototyping with the clustered GCN, if you have any
suggestions on this it would be very appreciated!

That sound very interesting!
Another approach that is inductive and so generalises to unseen graphs would be using GraphSAGE: https://github.com/stellargraph/stellargraph/blob/master/demos/node-classification/graphsage/graphsage-cora-node-classification-example.ipynb
This would definitely require putting everything into a single graph StellarGraph. However, unlike Cluster-GCN, this shouldn't suffer from any sparsity problems: for each training node, a sample is taken around it directly, so it doesn't care if there's many disconnected components in the overall graph.

Hi Huon,
I'm more than happy to share my code, please see attached. The code should
be able to replace the original mini_batch_node_generator.py as it wraps a
graph into a list if it is not one, but currently I'm only using it in my
project dir without modifying the original file. The code itself cannot
generalize for graphs with different sizes. I guess the generalization can
be done in the graph generation step, i.e. let all graphs to be generated
using the same list of node ids, (or be generated separately and mapped to
their union). This way some of the nodes will not be used by any edges for
smaller graphs. I'm not sure if it is the correct way to do it. Currently
my graphs are all the same size.
Thanks for your suggestion about using GraphSAGE. My choice of cluster-GCN
is due to its smaller memory cost. Some of my target CFD meshes have around
200M nodes, and after some literature review it seems that cluster GCN is
the only feasible option on my limited computing resources. But I'm very
new to graph neural networks, and I could be totally wrong on this. Please
kindly correct me if so.
Thanks,
-David
On Sun, Apr 19, 2020 at 9:27 PM Huon Wilson<EMAIL_ADDRESS>wrote:

From your description, that sounds like a very reasonable work-around. If
you're comfortable sharing the code, we might even be able to put it into
the library.
One drawback I realize is that this way is that in each batch the cluster
selected for all cases are the same. Do you think there are other problems
with it?
I'm not sure how well it generalises to graphs with different sizes, where
the clusters won't all have the same number of elements. Are your graphs
all the same size?
The problem I'm trying to address is to predict pressure distribution on
different CFD meshes. So the input features are x,y coordinates as well as
a distance function that defines the distance between each point and the
wall, the target I want to predict is the pressure (a scalar value on each
point). I'm still prototyping with the clustered GCN, if you have any
suggestions on this it would be very appreciated!
That sound very interesting!
Another approach that is inductive and so generalises to unseen graphs
would be using GraphSAGE:
https://github.com/stellargraph/stellargraph/blob/master/demos/node-classification/graphsage/graphsage-cora-node-classification-example.ipynb
This would definitely require putting everything into a single graph/
StellarGraph. However, unlike Cluster-GCN, this shouldn't suffer from any
sparsity problems: for each training node, a sample is taken around it
directly, so it doesn't care if there's many disconnected components in the
overall graph.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stellargraph/stellargraph/issues/1275#issuecomment-616262257,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AFZJFPUFSDGVR6C36JGYYWLRNOQJBANCNFSM4ML5GEEQ
.

I'm more than happy to share my code, please see attached

Great!
Unfortunately attachments don't come through the GitHub email -> comment conversion; could you post it some other way? For instance, if it's a single file (or only a few), one could put it in a gist. (If it's multiple files, one option would be to create a zip file and attach it to a comment via the GitHub UI.) We have no desire for a particular method, so whatever works for you :smile:

I took a slightly different approach from what I described above, because it seems that the graphconvolution layer specifies the input to be of size (1, ...) and I'm not sure if that's required for performance reasons or just an implementation limitation

It's an implementation limitation. Coincidentally, it has actually been lifted just today, in the latest develop version (via #1205). If you wish to use this new GraphConvolution layer from the library, you can install develop either directly with pip install git+https://github.com/stellargraph/stellargraph.git or by cloning the repo. Alternatively, you can just copy the new version out of the source, or wait a few days because we're planning to do release this week.

The code itself cannot
generalize for graphs with different sizes. I guess the generalization can
be done in the graph generation step, i.e. let all graphs to be generated
using the same list of node ids, (or be generated separately and mapped to
their union). This way some of the nodes will not be used by any edges for
smaller graphs. I'm not sure if it is the correct way to do it.

Hm, interesting. That's potentially a good route. I guess we'd have to sit down and actually implement to be sure.

Currently my graphs are all the same size.

Makes sense. Just so I understand, are you likely to be working with lists of graphs of different sizes in future?

Thanks for your suggestion about using GraphSAGE. My choice of cluster-GCN
is due to its smaller memory cost. Some of my target CFD meshes have around
200M nodes, and after some literature review it seems that cluster GCN is
the only feasible option on my limited computing resources. But I'm very
new to graph neural networks, and I could be totally wrong on this. Please
kindly correct me if so.

Yeah, you're correct that GraphSAGE results in a multiplicative increase in the amount of features that have to be stored in memory. However, the explosion is alleviated in StellarGraph by being a mini-batch method similar to Cluster-GCN; it batches up the requested "head" nodes, and only samples the neighbours (and fetches the features) for a small number of batches at a time.

Hi ,
Thanks for your reply. I noticed that the email does not carry the
attachments  correctly and modified my answer directly on the github issue
page so you can find a link to the code now. I also modified the comment a
bit, where I mentioned a problem about calling the on_epoch_end method, I'm
not sure if that's a real problem or just my implementation mistake.
in the future I'm likely to work on graphs with different sizes, they may
not be too different from each other though. Maybe a variation of 2~3%. But
even this will lead to many difficulties in the training I assume, although
I can't foresee any of the details.
Actually after some more literature review today, I realized that Cluster
GCN is more like a training treatment  on top of other modifications to the
convolution operation itself. I think what you described actually has all
the essences of Cluster GCN. Do you think there is any substantial
difference between them? Have you considered making clustering an option
for all methods, if it is not yet?
Thanks again for the detailed reply and all the work!
On Mon, Apr 20, 2020 at 6:03 AM Huon Wilson<EMAIL_ADDRESS>wrote:

I'm more than happy to share my code, please see attached
Great!
Unfortunately attachments don't come through the GitHub email -> comment
conversion; could you post it some other way? For instance, if it's a
single file (or only a few), one could put it in a gist
https://help.github.com/en/github/writing-on-github/creating-gists#creating-a-gist.
(If it's multiple files, one option would be to create a zip file and attach
it to a comment
https://help.github.com/en/github/managing-your-work-on-github/file-attachments-on-issues-and-pull-requests
via the GitHub UI.) We have no desire for a particular method, so whatever
works for you 😄
I took a slightly different approach from what I described above, because
it seems that the graphconvolution layer specifies the input to be of size
(1, ...) and I'm not sure if that's required for performance reasons or
just an implementation limitation
It's an implementation limitation. Coincidentally, it has actually been
lifted just today, in the latest develop version (via #1205
https://github.com/stellargraph/stellargraph/pull/1205). If you wish to
use this new GraphConvolution layer from the library, you can install
develop either directly with pip install git+
https://github.com/stellargraph/stellargraph.git or by cloning the repo
https://github.com/stellargraph/stellargraph#install-stellargraph-from-github-source.
Alternatively, you can just copy the new version out of the source
https://github.com/stellargraph/stellargraph/blob/eac335d472282b9d8b5dc72a41ce805e031d9f01/stellargraph/layer/gcn.py#L28-L218,
or wait a few days because we're planning to do release this week.
The code itself cannot
generalize for graphs with different sizes. I guess the generalization can
be done in the graph generation step, i.e. let all graphs to be generated
using the same list of node ids, (or be generated separately and mapped to
their union). This way some of the nodes will not be used by any edges for
smaller graphs. I'm not sure if it is the correct way to do it.
Hm, interesting. That's potentially a good route. I guess we'd have to sit
down and actually implement to be sure.
Currently my graphs are all the same size.
Makes sense. Just so I understand, are you likely to be working with lists
of graphs of different sizes in future?
Thanks for your suggestion about using GraphSAGE. My choice of cluster-GCN
is due to its smaller memory cost. Some of my target CFD meshes have around
200M nodes, and after some literature review it seems that cluster GCN is
the only feasible option on my limited computing resources. But I'm very
new to graph neural networks, and I could be totally wrong on this. Please
kindly correct me if so.
Yeah, you're correct that GraphSAGE results in a multiplicative increase
in the amount of features that have to be stored in memory. However, the
explosion is alleviated in StellarGraph by being a mini-batch method
similar to Cluster-GCN; it batches up the requested "head" nodes, and only
samples the neighbours (and fetches the features) for a small number of
batches at a time.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stellargraph/stellargraph/issues/1275#issuecomment-616444799,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AFZJFPS2HRMEJXRKAZ7HO3DRNQMWHANCNFSM4ML5GEEQ
.

I noticed that the email does not carry the
attachments  correctly and modified my answer directly on the github issue
page so you can find a link to the code now.

Thanks, it looks like a reasonable approach given the constraints you have.

I also modified the comment a
bit, where I mentioned a problem about calling the on_epoch_end method, I'm
not sure if that's a real problem or just my implementation mistake.

Nice catch! It looks like it's a Tensorflow problem, that may be fixed in TF 2.2 (based on the mention of it being fixed in tf_nightly a while ago): https://github.com/tensorflow/tensorflow/issues/35911
I've opened https://github.com/stellargraph/stellargraph/issues/1356 to make sure that StellarGraph handles it properly and validates it is fixed.

in the future I'm likely to work on graphs with different sizes, they may
not be too different from each other though. Maybe a variation of 2~3%. But
even this will lead to many difficulties in the training I assume, although
I can't foresee any of the details.

The problem I see is: the approach requires every graph to have batches in exactly the same shape, this means the clusters have to have the same sizes. If the graphs have different numbers of nodes, there will be some clusters that have more nodes; for instance, with 2 clusters, a graph of size 6 has two clusters of size 3, while a graph of size 7 has a cluster of size 3 and one of size 4. These can't be batched together directly because the tensor shapes don't line up.
One work around for this would be to ensure all the graphs have the same size by padding some isolated nodes onto the smaller ones, to match the largest graph.

Actually after some more literature review today, I realized that Cluster
GCN is more like a training treatment  on top of other modifications to the
convolution operation itself. I think what you described actually has all
the essences of Cluster GCN. Do you think there is any substantial
difference between them? Have you considered making clustering an option
for all methods, if it is not yet?

That's a great point. I'm not sure it's been considered, so I've opened another issue (https://github.com/stellargraph/stellargraph/issues/1355) to track this specifically.

I guess one risk is that training may be inefficient, because there will be a lot clusters that have nodes from many graphs, and not many edges will be being considered.

Ah, going back to this. I forgot that there's a way to improve this, just FYI. The ClusterNodeGenerator(..., clusters=...) argument supports passing in the list of clusters. https://stellargraph.readthedocs.io/en/stable/api.html#stellargraph.mapper.ClusterNodeGenerator

clusters (int or list) – If int then it indicates the number of clusters (default is 1 that is the given graph). If clusters is greater than 1, then nodes are uniformly at random assigned to a cluster. If list, then it should be a list of lists of node IDs such that each list corresponds to a cluster of nodes in G. The clusters should be non-overlapping.

This would allow for creating a single huge graph with all of the meshes as disconnected subgraphs, and then have each cluster only contains nodes from a single graph. For example:
nodes_per_mesh = ... # a list of lists of nodes for each graph
num_clusters = ...

clusters = [
    nodes[idx::num_clusters] # every num_clusters'th node from this mesh
    for nodes in nodes_per_mesh
    for idx in range(num_clusters)
]

assert len(clusters) == len(nodes_per_mesh) * num_clusters

This still has problems for q > 1, since each group of q clusters is unlikely to have many from the same graph, and so there's no edges between clusters in the group, only edges within each cluster.

Hey @davidxujiayang, you may've noticed my comment on #1355: now the APPNP and GAT (and GCN) support taking a ClusterNodeGenerator in addition to a FullBatchNodeGenerator and so can be trained with Clusters (#1531, #1585).


chore(main): release 2.0.6

Auto generated release
2.0.6 (2024-05-14)
Miscellaneous Chores

deps: update solid/start to 1.0.0-rc.1 (#138) (fcf58f8)

This PR was generated with Release Please. See documentation.

:robot: Release is at https://github.com/jobtrek/grades-view-solid/releases/tag/v2.0.6 :sunflower:


Replace unsupported build-base directive

When building blockdiag, the following warning appears:

Usage of dash-separated 'build-base' will not be supported in future
versions. Please use the underscore name 'build_base' instead.

By 2024-Sep-26, you need to update your project and remove deprecated calls
or your builds will no longer be supported.

This patchs fixes it by switching build-base to build_base.

Thanks!