Options appear to be ignored

I just wanted to let you know that I've (only very briefly, without doing any further debugging) tried this feature out, for https://github.com/google/android-fhir/issues/2481, and it seems to me that the Options to chose the Android SDK version appear to be ignored?
In https://github.com/google/android-fhir/pull/2482 (here) I originally tried:
 "ghcr.io/nordcominc/devcontainer-features/android-sdk:1": {
      "platform": "33",
      "build_tools": "33.0.1"
    }

and in the creation.log it (correctly) noticed:
#16 0.762 Feature       : Android SDK2024-03-15 23:05:49.933Z:
#16 0.762 Description   : Install Android SDK cmdline-, platform-, and, build-tools
#16 0.762 Id            : ghcr.io/nordcominc/devcontainer-features/android-sdk
#16 0.762 Version       : 1.1.0
#16 0.762 Documentation :
#16 0.762 Options       :
#16 0.762     PLATFORM="33"2024-03-15 23:05:50.082Z:
#16 0.762     BUILD_TOOLS="33.0.1"

but then it still installed v34 instead of v33:
[====                                   ] 10% Downloading build-tools_r34-linux.
[=======                                ] 18% Unzipping... android-14/NOTICE.txt

I've noticed that several folks published Dev Container Features for installing the Android SDK, so I'll try out another one. I'm only filing this issue to let you know.

Thanks, looks like a simple shell script mistake managed to slip under the radar. I'll take a look at it over the weekend! :)

Appears that somebody already had investigated, I'll do a proper check later to verify but it should/might be working now!

I just wanted to let you know that I've (only very briefly, without doing any further debugging) tried this feature out, for google/android-fhir#2481, and it seems to me that the Options to chose the Android SDK version appear to be ignored?
In google/android-fhir#2482 (here) I originally tried:
 "ghcr.io/nordcominc/devcontainer-features/android-sdk:1": {
      "platform": "33",
      "build_tools": "33.0.1"
    }

and in the creation.log it (correctly) noticed:
#16 0.762 Feature       : Android SDK2024-03-15 23:05:49.933Z:
#16 0.762 Description   : Install Android SDK cmdline-, platform-, and, build-tools
#16 0.762 Id            : ghcr.io/nordcominc/devcontainer-features/android-sdk
#16 0.762 Version       : 1.1.0
#16 0.762 Documentation :
#16 0.762 Options       :
#16 0.762     PLATFORM="33"2024-03-15 23:05:50.082Z:
#16 0.762     BUILD_TOOLS="33.0.1"

but then it still installed v34 instead of v33:
[====                                   ] 10% Downloading build-tools_r34-linux.
[=======                                ] 18% Unzipping... android-14/NOTICE.txt

I've noticed that several folks published Dev Container Features for installing the Android SDK, so I'll try out another one. I'm only filing this issue to let you know.

I can confirm that it's working now using the configuration you originally tried with!

Sorry about miss there! This devcontainer feature was originally created by me to play a little bit with Compose without having to spin up a full-blown Android environment locally, but there hasn't been too much time available for that, so this ended up mostly lying dormant 😅
Let me know if any other issues arise!


😇

@msavin I would love to add your project, but why instead, maybe just add your as the first on top (as yours is obviously better maintained)

addressed in newer PRs


webhook trigger

very good tile

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

ID: 9255b6c5-d435-f89b-d813-cf85c5326041
Version Independent ID: cbb10e07-56dd-99ba-d48f-97e48386dc52
Content: Create a function in Azure triggered by a GitHub webhook
Content Source: articles/azure-functions/functions-create-github-webhook-triggered-function.md
Service: functions
GitHub Login: @ggailey777
Microsoft Alias: glenga

Thanks for the feedback :)


diagonal shooting buggy

It appears that you can only shoot diagonally when moving upper-right.

I'm gonna try and find the reason for the Bug, I'm learning :-)


Use absolute path for entrypoint

If I run the docker run command verbatim from the old readme i get an error.
docker: Error response from daemon: Container command 'concourse-web.sh' not found or does not exist..

It seems you are right, the docker-compose file and the scripts use absolute paths as well. Thanks for fixing.


Ignore test generated output

One line added to gitignore file to ignore generated files in project's out/ directory. The directory is populated when tests are run.

I'm curious, how are you running the tests? Running the tests does not create an out/ folder for me.
Is this an artifact of an IDE you are using or something else?
I have no objections to ignoring it, but I just want to make sure there isn't some error happening.

Mystery solved! I'm using Intellij which has a plugin to analyze code smells. The plugin is responsible for generating the out/ folder.


feat(android): add compatible showPayPalViewController function

Hey there, I added a Android version of the showPayPalViewController function.

Thanks!

Thanks for the quick reaction!


📘 Improve existing documentation. Add project structure and note about Netlify CMS

Adds line explaining to set the Github token to the environment variable
NPM_TOKEN

@samuelpearce, does this documentation look good to you now?

@viktor-yakubiv This is brilliant. Much more than I was expecting. The diagram and explanation of how the different libraries link together and other hidden dependencies (e.g. API) are really useful. Thanks


Please update glow to v1.2.1

Comment:
Dear @kianfar77, would it be possible to automatically publish the latest glow.py releases here?
I currently cannot update to the latest glow version because the PyPI package would break my enviroment by overwriting certain conda packages.

@Hoeze Apologies for the long delay. v2.0.0 should be available in conda-forge soon.

Thanks a lot for the update @henrydavidge!


extracellular times-recording unnecessary

The fix in #427 also includes a removal of the arbitrary one-sample-mismatch between the length of the potential and dipole traces. The latter are one short because extra_scatter_gather is only called after a solver step. The solution was to set the initial state to 0 mV (which I suppose is debatable).
Presently _ExtracellularArrayBuilder creates an independent copy of h.t for every array attached to the network:
self._nrn_times = h.Vector().record(h._ref_t)

This is redundant, since h._ref_t is already recorded in network_builder.py:_simulate_single_trial(). One simple solution is to let arr._times = sim_data[0]['times'] in the below (parallel_backends.py:_gather_trial_data())
        # extracellular array
        for arr_name, arr in net.rec_arrays.items():
            # voltages is a n_trials x n_contacts x n_samples array
            arr._data.append(sim_data[idx]['rec_data'][arr_name])
            arr._times = sim_data[idx]['rec_times'][arr_name]

Do I get a +1 for this? And in the bigger picture, do we want keep having times embedded in each of the various output objects we create (dpl, rec_arr, CellResponse)?

Sounds good to me. Idk why we need redundant copies of times passed around if they are all the same.

agree there should only be one "master" times. Not sure if the initial state should be included? I think having times in the different objects is fine for convenience with plotting etc? We just make sure it's set only in _gather_trial_data so they are all equal. Could even have a test that checks for this

0.2 milestone @cjayb ?

We just make sure it's set only in _gather_trial_data so they are all equal.

Yeah this seems like the easiest approach that's still convenient (plotting). I hesitate because it causes an unnecessary copy, but times isn't something to worry about, I guess.

Not sure if the initial state should be included?

True, especially since our initial state is somewhat poorly defined.

0.2 milestone @cjayb ?

I have a suspicion this might balloon a bit, possibly interact with the pending discussion on initial 'transients' etc. Let's leave this Issue open for now and see what that other topic churns up?

Can I work on this issue @cjayb @jasmainak @rythorpe ?

Hey Raj, I'm afraid I'm quite useless here as I haven't looked at the code since last year :( I don't know the relevance of this Issue, but it is labeled as a good first issue, @jasmainak @rythorpe ?

One issue at a time @raj1701 ... let's first see your other PRs merged


uv sync and uv run --no-project end up in different version with same requirements

Hello
I had an issue with torch dependency. I am pretty sure the problem comes from torch and I opened an issue on that matter: https://github.com/pytorch/pytorch/issues/141781
However, while investigating my issue I encountered a uv behaviour I do not fully understand.
The issue can be reproduced with the following very simple pyproject.toml:
[project]
name = "uv-torch-test"
version = "0.1.0"
requires-python = ">=3.8"
dependencies = [
    "torch",
]

If you uv sync with Python 3.8 (either using .python-version or --python 3.8 option), it fails with the following error
error: Distribution `torch==2.5.1 @ registry+https://pypi.org/simple` can't be installed because it doesn't have a source distribution or wheel for the current platform

I think this is expected given the issue described here: https://github.com/pytorch/pytorch/issues/141781.
However, the following command succeeds and indicates that uv ends up installing torch 2.4.1 in this case
uv run --no-project --python 3.8 --with "torch" python -c "import torch; print(torch.__version__)"

In conclusion:

One feature request:

Having an option to print the package versions in uv run --no-project the same way they are printed in uv sync would have helped me a lot. The only solution to print them today is to use the --verbose option which is extremely verbose. I use a lot the "uv run --no-project" command in my unit testing CI jobs, and adding the --verbose would make the test results and logs difficult to read, whereas I just need the package versions.

Two questions:

Why does uv run --no-project --python 3.8 --with "torch" dependency resolution ends up choosing 2.4.1 and not 2.5.1 when 2.5.1 is identified as compatible with Python 3.8 by uv sync?
Would #8686 help avoid this behaviour? (really waiting for this one, it would help on several issues I encountered 😉 )

Thank you 🙏

Hello
Quick update after some additional testing.
pip install torch in a blank Python 3.8 environment installs 2.4.1, which indeed is the real last Python 3.8 compatible torch release, despite what the metadata of the version says. This leads me to two conclusions:

pip does not respect its own interface, in the sense that it does not fail if a package metadata is not properly written (in that torch 2.5.0/2.5.1 case, the package does not provide the wheels or sdist for all supposedly compatible Python versions), which bugs me a lot as a developer 😱
I am not sure about that, but I think what pip does under the hood is that it looks at the available wheels or sdist rather than looking at the requires_python specifications (I looked a bit at the pip sources but I did not find where this is done yet).

On the uv side:

uv sync seems to resolve the dependencies by looking at the packages metadata, which totally makes sense to me, and the error I got definitely comes from a problem in the PyTorch releases and is therefore fully legit I think it is a good think uv reports it as an error.
I still do not understand why uv run --no-project --python 3.8 --with "torch" does not fail. Does it use pip resolver under the hood?

In general, I wanted to say that uv sync resolver seems really excellent to me as its rigorous "fail-fast" behaviour helped me spot errors in packages metadata really easily.

The difference is that uv sync is doing a "universal" solve, whereas uv run --with only has to solve for your current platform / environment, since it's not creating a universal lockfile.

(Happy to answer any further questions.)

I'm not fluent enough in rust to look at the uv code, but I tried to interpret the uv verbose logs.
I'm not sure what to interpret from these 😅
--
Here is (part of) the output of uv lock --python 3.8 --verbose with the pyproject.toml of my initial message:
DEBUG uv 0.5.5 (Homebrew 2024-11-27)
[...]
DEBUG Using Python request `3.8` from explicit request
[...]
Using CPython 3.8.20
[...]
DEBUG Solving with installed Python version: 3.8.20
DEBUG Solving with target Python version: >=3.8
[...]
DEBUG Adding transitive dependency for uv-torch-test==0.1.0: torch*
DEBUG Found fresh response for: https://pypi.org/simple/torch/
DEBUG Searching for a compatible version of torch (*)
DEBUG Selecting: torch==2.5.1 [compatible] (torch-2.5.1-cp310-cp310-manylinux1_x86_64.whl)

--
Here is (part of) the output of uv run --no-project --verbose --python 3.8 --with "torch" python:
DEBUG uv 0.5.5 (Homebrew 2024-11-27)
[...]
DEBUG No project found; searching for Python interpreter
DEBUG Searching for Python 3.8 in virtual environments, managed installations, or search path
[...]
DEBUG Using Python 3.8.20 interpreter at: [...]/.local/share/uv/python/cpython-3.8.20-macos-aarch64-none/bin/python3.8
[...]
DEBUG Solving with installed Python version: 3.8.20
DEBUG Solving with target Python version: >=3.8.20
DEBUG Adding direct dependency: torch*
DEBUG Found fresh response for: https://pypi.org/simple/torch/
DEBUG Searching for a compatible version of torch (*)
DEBUG Searching for a compatible version of torch (<2.5.1 | >2.5.1)
DEBUG Searching for a compatible version of torch (<2.5.0 | >2.5.0, <2.5.1 | >2.5.1)
DEBUG Selecting: torch==2.4.1 [compatible] (torch-2.4.1-cp38-none-macosx_11_0_arm64.whl)

--

One more experiment.
I compiled the version from https://github.com/astral-sh/uv/pull/8686 and ran
uv lock --python 3.8 --multi-version lastest

on the pyproject.toml from my initial message.
=> uv still resolves torch 2.5.1 for all Python versions (3.8 to 3.13)
=> uv still fails to sync with Python 3.8 as it tries to install torch2.5.1 (same error as my first message).
So unfortunately, https://github.com/astral-sh/uv/pull/8686 won't help in that case and the mystery of why my two commands do not behave the same way is still bothering me a bit 🙂
I can provide logs if that can help.

We look at the requires-python, not the available wheels. We look at the wheels to determine whether there's at least one wheel in the space of supported Python versions. The assumption is that other versions will build from source. PyTorch doesn't ship source, so you end up with a version that may not support (e.g.) Python 3.8. Hence the open issue that I linked earlier.

Thank you, absolutely all clear, now I understand why the two commands end up with different versions 👌
This totally makes sense and is quite clever to get a fast resolution while taking advantage of PyPI API.
I must say that the thing that bugs me the most here is that nothing happened in both torch release process and PyPI backend when faced to what seems to me two ill-formed releases, in the sense that the requires-python is not respected by the wheels or sdists 😮
It is quite funny how uv helped me spot several eronous dependency specs in packages really quickly (see https://github.com/astral-sh/uv/issues/9211#issuecomment-2484266926).
For me that is a huge advantage of uv, this is the first time I find such an efficiently rigorous (which I love) environment manager in the Python world 👏

@charliermarsh
FYI: the problem on PyTorch side is being taken care of: https://github.com/pytorch/pytorch/issues/141781.
No sure when, but this (and https://github.com/astral-sh/uv/issues/5182 if I understood well) should be fixed in the near future, without having to specify complex version constraints in order to fix in uv what is in fact a bug on PyTorch side.


Using the Orphanage

There is a way for use it?
I configured the orphanage following the OneupUploadBundle instructions, but when I try to upload a file, the JsonResponse is empty, but the file was uploaded.
Thanks.

No, this bundle cannot be used with the orphenage function of the openup bundle.
It does not use the orphenage manager when handling the upload :
https://github.com/1up-lab/OneupUploaderBundle/blob/master/Resources/doc/orphanage.md#the-controller-part

Which would be the best way to use it?

I don't think it will be easy.
If you don't use the crop tool, you should be able to use it by configuring the storage to be orphanage. It should work. You will maybe have to configure your own OrphenageResolver to generate a temporary url when the file is in the orphenage storage.
If you use the crop tools, this will be harder as the crop is done in 2 actions : the classic upload one and a custom crop controller.
It was a deliberate choice to not use the orphanage. Too much trouble to implement and disk space is cheap.
Note : all uploaded files have an entry in the history table. You could use it to find which files need to be removed.

I try to use the orphanage with the jb_image_ajax field type, but the ajax response is empty.
Every file uploaded have a entry in the history table, inclusive the files that are no used, suppose you upload a file, this file is inserted in the history table, but you selected a wrong image, and you upload a new image, the second image is inserted in the history table too, what happened with the first image? How I detect or control that the first image is not valid.
Thanks.

If the response is totally empty, you most probably have a PHP error. You should check the log files of SF or your webserver.
To answer your question, every single upload is tracked in the history table. Nothing is ever deleted right now. This is not a doctrine event but an extension of the oneup upload manager which inject in the table.
As I said previously, right now, there is no detection that the first image is no longer valid. If you don't succeed in using the orphanage, I have thought of an idea : if only authenticated user can upload, you could use a single oneup mapping per upload form type in your project and add a unique constraint on the history table for type (which store the mapping used) and user_id field. That way, if you have a non unique exception it means that the user is trying to upload a new file for the same field.

This is the SF log error using the orphanage and the jb_image_ajax field type.
[2015-10-07 19:22:25] event.INFO: An exception was thrown while getting the uncalled listeners. {"exception":"[object] (Symfony\\Component\\DependencyInjection\\Exception\\InactiveScopeException(code: 0): You cannot create a service (\"jb_fileuploader.file_ajax.upload_listener\") of an inactive scope (\"request\"). at /Users/ranacona/Sites/symfony_demo/app/cache/dev/appDevDebugProjectContainer.php:1835)"} []

So, the history table cannot be used for check the use files and ones not. I said this, because the history table is filled immediately the file is uploaded and not after the entity is persisted.
I can't use the last option because a user (user_id: 3) can upload a image to many different records.

Very strange. You error you are getting is a container scope error. But the manager is called when uploading an image so in a request scope. You should not have this error.
Another idea : upload files in a directory based on the current date. And check at the end of each day which files are used or not. It is kind of hacky but it could work. But this bundle does not handle the orphanage right now. As you have pointed out, it use the onUpload event to fill in the history table.


Update costs

This change is

The costs updates happen due to the OS changes here:
https://reviewable.io/reviews/starkware-industries/starkware/35349#-


x/mobile: binding Gosseract fails

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

$ go version
go version go1.17 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
GO111MODULE=""
GOARCH="amd64"
GOBIN="/home/dpanic/go/bin"
GOCACHE="/home/dpanic/.cache/go-build"
GOENV="/home/dpanic/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/dpanic/go/pkg/mod"
GOOS="linux"
GOPATH="/home/dpanic/go"
GOPROXY="https://goproxy.io,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.17"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1397584906=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I am trying to bind Go code for Android, code has CGO bindings for Tesseract.
I have all dependencies installed for tesseract.
I am trying to archive this with gomobile bind but I get following error:
$ gomobile bind
gomobile: go build -buildmode=c-shared -o=/tmp/gomobile-work-1045436346/android/src/main/jniLibs/armeabi-v7a/libgojni.so ./gobind failed: exit status 2
# github.com/otiai10/gosseract
tessbridge.cpp:5:10: fatal error: 'tesseract/baseapi.h' file not found

What did you see instead?
tessbridge.cpp:5:10: fatal error: 'tesseract/baseapi.h' file not found

This is an issue with your install of tesseract, see their issue tracker
Unlike many projects, the Go project does not use GitHub Issues for general discussion or asking questions. GitHub Issues are used for tracking bugs and proposals only.
For questions please refer to https://github.com/golang/go/wiki/Questions


Unregister all broker metrics on broker stop

Add methods for registering broker-specific metrics so it is possible to
keep track of them and they can be unregistered on broker stop.
Fixes #1219

I think there can still be another leak with topic metrics, as they are never unregistered, I'll take a look to this as a follow up.

Can you please look into writing a test around it?

Thank you!


Update documentation for onChange property

Currently the released version requires onChange property, but it's not mentioned in the documentation why and what should be placed there.

Hi, I've added an example in the readme:
onChange = (crop) => {
  this.setState({ crop });
}

Hi @DominicTobias, thanks for the fast response. I just thought this is not really required, is it? The PropType is marked .isRequired... or is that a wanted API change? I just don't know and I recognized that it wasn't before ;)
If it's a wanted change, just go ahead and close the ticket :)

Yep it's a wanted change, since the crop state is no longer stored in the ReactCrop component ;)

PS I will check out your importing issue later


Non leaders of Coordinator and Overlord should proxy HTTP requests to the leader.

Description
Non leaders of Coordinator and Overlord should proxy all HTTP requests to the leader and proxy back the HTTP response instead of performing redirections.
Motivation
If this feature exists, end users can put a regular load balancer such as Nginx on top of Coordinator and Overlord.

Druid's router routes requests to leader coordinator and overlord, but this requires router deployed. Since router is optional, such proxy from non-leader nodes to leader nodes makes sense.


However, like I said I don't know how many times now, the problem isn't just with performance, it's also about usability. You're providing very little data on how users would actually go about using this in practice. Like, it's not obvious how one would pass a scope from one object to another, without having it get closed prematurely. How do you even do that? It's supposed to be a very simple operation: pass a reference, don't call close(), and we're done! You're making everything more complicated for no good reason, that I can see. In my opinion, your API isn't user-friendly. Please prove me wrong. If you're confident about this magical paradigm of yours that comes from nowhere but that you want to force onto everyone, you'll need to provide something to reason about.

See upthread, I'm already going to convert it to something close to Panama.

It polls the physical memory used by the process to compare against maxPhysicalBytes. It also keeps tracks in Pointer.totalBytes of the capacity of all allocated Pointer object, which gets compared against the other maxBytes threshold, but if not set from an allocation in normal C++, it needs to be set manually, yes. Overriding sizeof() probably makes sense here, yes, because since TF_Tensor is an opaque type, it doesn't have any size anyway.

Yeah I saw maxPhysicalBytes, but it wasn't working for me.  Windows would move stuff to the swap file, which meant I kept getting more physical memory, until it OOM'd (I think because of the swap file size limit).  But as long as I override the size, the user can choose to use maxBytes or maxPhysicalBytes.

I don't see how this could be meaningfully extended to other devices, performance is already pretty abysmal just for buffers on CPU as it is, but that's a discussion for another time.

What do you mean about performance?  Is the reference tracking costly?  What I meant with devices is that eager operands may live on non-CPU devices (i.e. a GPU) and their memory use needs to be tracked with respect to that device.  Afaik our tensor implementation is CPU-only though, so yeah, it's irrelevant here.

I've updated things to use the scope passing style now, I think we have broad agreement that's the way to go (and it's what Panama uses).  Still probably not worth trying to get it in 0.3.0.

I've updated things to use the scope passing style now, I think we have broad agreement that's the way to go (and it's what Panama uses).  Still probably not worth trying to get it in 0.3.0.

@karllessard is the CI down again?  I'm trying to build with -Pdev (to rebase) but keep getting:
Could not resolve dependencies for project org.tensorflow:tensorflow-core-platform:jar:0.3.0-SNAPSHOT:
The following artifacts could not be resolved:
    org.tensorflow:tensorflow-core-api:jar:linux-x86_64:0.3.0-SNAPSHOT,
    org.tensorflow:tensorflow-core-api:jar:macosx-x86_64:0.3.0-SNAPSHOT:

Could not find artifact org.tensorflow:tensorflow-core-api:jar:linux-x86_64:0.3.0-20210211.071905-248 in ossrh-snapshots (https://oss.sonatype.org/content/repositories/snapshots) -> [Help 1]

The windows artifact is in the snapshot repository but the mac and linux versions only have mkl builds.

I think the changes are already done, I'll check again though.  I need to do a docs update, too.

Ok let me know when you are done and I'll merge this.
Suggestion: depending on their implementation, it could be tricky for users to use Operand.tensor(scope) now that Operand.tensor() is gone. I'm wondering if it wouldn't be best to restore that previous one (which defaults to the eager session) and deprecate it, at least for this next release, wdyt?

Ok let me know when you are done and I'll merge this.
Suggestion: depending on their implementation, it could be tricky for users to use Operand.tensor(scope) now that Operand.tensor() is gone. I'm wondering if it wouldn't be best to restore that previous one (which defaults to the eager session) and deprecate it, at least for this next release, wdyt?

Yeah, that sounds like a good idea, especially if we end up adding operand scoping and making it use that.

So @rnett , it looks like we are good to merge this PR, or were you planning to add/change something from the previous feedbacks? You'll need to rebase it though.

I keep telling you guys that this doesn't work when noPointerGC is enabled... Please fix this.

Ok let me know when you are done and I'll merge this.
Suggestion: depending on their implementation, it could be tricky for users to use Operand.tensor(scope) now that Operand.tensor() is gone. I'm wondering if it wouldn't be best to restore that previous one (which defaults to the eager session) and deprecate it, at least for this next release, wdyt?

Yeah, that sounds like a good idea, especially if we end up adding operand scoping and making it use that. There's probably a few other spots where that would be helpful too.

BTW, since the whole of TF Core was designed with C++ and Python in mind, I think we should be able to share "scopes" between different types of objects. That's what I'm doing with PointerScope for C++ APIs in general, and if it looks like we'll need to do the same kind of thing with TF, we should have some kind of "scope" that is more general as well. I don't think it would be a good idea to have separate "TensorScope", "OpScope", "WhateverOtherScope", etc.

I agree that TensorScope could be made more generic and renamed to something like ResourceScope. It looks like the only dependency on Tensor inside it is to detach/attach a tensor to its own internal scope when attaching/detaching it to another scope (btw why having an internal scope in Tensor and not simply a reference to its Pointer?)
If we really need access to that internal pointer/scope, we could define a new interface type (NativeResource?) that extends from AutoCloseable and exposes some generic methods like Pointer pointer(), PointerScope pointerScope(), etc. to become completely independent from Tensor inside ResourceScope. Normally, that could be reused as is for other native resources we would like to manage with scopes in the future, like eager operands, even if this is encapsulated by the Ops.
@rnett wdyt? I know this could kind of slow down a bit again the merge of this PR but memory management is really on the critical path of TF Java and we better take our time for doing it well, let me know if you need help.

I agree that TensorScope could be made more generic and renamed to something like ResourceScope. It looks like the only dependency on Tensor inside it is to detach/attach a tensor to its own internal scope when attaching/detaching it to another scope.

This is doable, but the Resource interface would have to expose it's current scope for detaching, which is something we may not want as a public API.  It would need to be settable for attaching as well, which I really don't like exposing.

(btw why having an internal scope in Tensor and not simply a reference to its Pointer?)

You mean to the PointerScope?  I just left it that way, I presume it's because we don't forbit creating multiple RawTensor objects from the same TF_Tensor.

If we really need access to that internal pointer/scope, we could define a new interface type (NativeResource?) that extends from AutoCloseable and exposes some generic methods like Pointer pointer(), PointerScope pointerScope(), etc. to become completely independent from Tensor inside ResourceScope. Normally, that could be reused as is for other native resources we would like to manage with scopes in the future, like eager operands, even if this is encapsulated by the Ops.

I don't actually need the PointerScope, but I do need the current ResourceScope for detaching (which needs to be settable) and a close method.  See my above comment.

@rnett wdyt? I know this could kind of slow down a bit again the merge of this PR but memory management is really on the critical path of TF Java and we better take our time for doing it well, let me know if you need help.

It's a pretty easy change if we're ok with exposing things and I agree it makes the code better, but the only other thing I foresee being tracked this way is eager operands, I don't think anything else is memory heavy enough.  Given that there's only two uses imo it's not worth having to expose the Resource's scope setters, unless someone can think of a way around that.

So @rnett , it looks like we are good to merge this PR, or were you planning to add/change something from the previous feedbacks? You'll need to rebase it though.

I keep telling you guys that this doesn't work when noPointerGC is enabled... Please fix this.

You mean because the weak refs will be dropped but the deallocation won't be done on the JavaCPP side?  My thinking is to make the weakness optional, with it being strong by default if noPointerGC is true.

I agree that TensorScope could be made more generic and renamed to something like ResourceScope. It looks like the only dependency on Tensor inside it is to detach/attach a tensor to its own internal scope when attaching/detaching it to another scope.

This is doable, but the Resource interface would have to expose it's current scope for detaching, which is something we may not want as a public API. It would need to be settable as well, which I really don't like exposing. We could do better with an abstract class but that has all other sorts of issues.

That's the kind of thing I've been arguing about. If we don't make attach()/detach() a part of the API, we don't need to worry about these things!

If we really need access to that internal pointer/scope, we could define a new interface type (NativeResource?) that extends from AutoCloseable and exposes some generic methods like Pointer pointer(), PointerScope pointerScope(), etc. to become completely independent from Tensor inside ResourceScope. Normally, that could be reused as is for other native resources we would like to manage with scopes in the future, like eager operands, even if this is encapsulated by the Ops.

I don't actually need the PointerScope, but I do need the current ResourceScope for detaching (which needs to be settable) and a close method. See my above comment.

If we don't allow detaching, we don't need to worry about that!

@rnett wdyt? I know this could kind of slow down a bit again the merge of this PR but memory management is really on the critical path of TF Java and we better take our time for doing it well, let me know if you need help.

It's a pretty easy change if we're ok with exposing things and I agree it makes the code better, but the only other thing I foresee being tracked this way is eager operands, I don't think anything else is memory heavy enough. Given that there's only two uses imo it's not worth having to expose the Resource's scope setters, unless someone can think of a way around that.

Would you still foresee the need to expose anything if we don't allow users to attach/detach stuff?

You mean because the weak refs will be dropped but the deallocation won't be done on the JavaCPP side? My thinking is to make the weakness optional, with it being strong by default if noPointerGC is true. Is that ever exposed anywhere?

There's that, but simply keeping weak references to pointers also doesn't work with GC anyway. The JVM needs at least 2 passes to clear weak and phantom references. We can (and will) lose access to weakly referenced pointers before we can get the chance to free them, deterministically, and then free the context they belong to, so memory will leak. We need to free the ops before the context gets freed. There is no way to enforce this ordering of events using GC. I think the original approach that was done for TF 1.x does work correctly tough, see https://github.com/tensorflow/java/pull/229#issuecomment-790310233:

Right, delete() and deleteAll() appear synchronized enough that calling them concurrently should be fine. Looking more closely, I think the only issue is when close() doesn't get called, the context itself never gets deleted. Anyway, if we wanted to do something similar to that with JavaCPP, I'd synchronize the deallocate() method here:
https://github.com/tensorflow/java/blob/master/tensorflow-core/tensorflow-core-api/src/main/java/org/tensorflow/internal/c_api/AbstractTFE_Op.java#L30
That gets assigned to Pointer.deallocator, which we can retrieve with the protected deallocator() method from those subclasses, keep strong references to them, and call deallocate() manually on them when desired. That should work well regardless of whether noPointerGC is enabled or not.

Would you still foresee the need to expose anything if we don't allow users to attach/detach stuff?

If we only ever assigned/attached to scopes on creation, we could get rid of these.  We'd need a copyToNewScope like you suggested.  That would essentially be reference counting though, so we might as well just go all the way.  Another point for reference counting over copying is that I'd like the tensor scope to match the operand scope and copying operands is not easily doable.  I'm gonna make another reply about that.

You mean because the weak refs will be dropped but the deallocation won't be done on the JavaCPP side? My thinking is to make the weakness optional, with it being strong by default if noPointerGC is true. Is that ever exposed anywhere?

There's that, but simply keeping weak references to pointers also doesn't work with GC anyway. The JVM needs at least 2 passes to clear weak and phantom references. We can (and will) lose access to weakly referenced pointers before we can get the chance to free them, deterministically, and then free the context they belong to, so memory will leak. We need to free the ops before the context gets freed. There is no way to enforce this ordering of events using GC. I think the original approach that was done for TF 1.x does work correctly tough, see #229 (comment):

Right, delete() and deleteAll() appear synchronized enough that calling them concurrently should be fine. Looking more closely, I think the only issue is when close() doesn't get called, the context itself never gets deleted. Anyway, if we wanted to do something similar to that with JavaCPP, I'd synchronize the deallocate() method here:
https://github.com/tensorflow/java/blob/master/tensorflow-core/tensorflow-core-api/src/main/java/org/tensorflow/internal/c_api/AbstractTFE_Op.java#L30
That gets assigned to Pointer.deallocator, which we can retrieve with the protected deallocator() method from those subclasses, keep strong references to them, and call deallocate() manually on them when desired. That should work well regardless of whether noPointerGC is enabled or not.

I'm still not entirely understanding this.  You're saying that we may have a tensor get GC'd and then have the scope closed, where the pointer deallocate and the scope closing deallocate go off at the same time because the weak reference isn't removed in time?  What are you referring to with "context"?  There's nothing that needs to be freed other than tensors.

cc @karllessard @Craigacp
For reference counting more generally, the motivating example for me is something like:
class KeepTensor(var tensor: Tensor?)

fun main(){
  val keep = KeepTensor(null)
  TensorScope().use{
    keep.tensor = makeTensor(this)
  }
  // keep.tensor is now closed
}

This is very contrived, but the example came from me wondering if ConstantInitializer could use tensors instead of the value type enum.  In cases where you want a tensor to match the object lifetime there's no good solution.  Consider if KeepTensor was implemented like:
class KeepTensor(tensor: Tensor?){
  private val scope = TensorScope()
  var tensor: Tensor? = tensor
    set(value) {
      field = value?.let(scope::attach)
    }
}

While this works in most cases, if we change our example to something like:
fun main(){
  val keep = KeepTensor(null)
  val keep2 = KeepTensor(null)
  TensorScope().use{
    Tensor temp = makeTensor(this)
    keep.tensor = temp
    keep2.tensor = temp
  }
}

This happens whenever a tensor is attached/detached by the caller, after being stored somewhere in a called function.  Since we only allow attaching to one scope, the tensor will only be attached to keep2's scope.  Now, if we had a copyInScope(TensorScope) method, it would nicely solve this, at least for pure tensors.  However it doesn't work well with collections or containers, or with Operands, and I would like the Operand scope to match the tensor one (at least, ideally they would be the same).  Reference counting is a better solution here, and is what copying would be doing anyways (via PointerScope).

Implementation wise, if we want to generify it to Resource (which I think is a good idea), Resource would have to track and expose reference counters, which I don't like as it has all the same problems as exposing the scope.  However, we could instead have scopes track their members and track all active scopes, using that to get the reference count instead.  If we make the scope tracking weak, this also nicely removes references from GC'd scopes, although it won't necessarily clean up their now 0-reference resources (that would require another Cleaner like object and thread, but it's a backstop anyways so imo it's not worth that.  Although if/when we add a Cleaner for something else we could use it here).  Given that we shouldn't have a huge amount of scopes open, this should be fairly cheap (it's linear wrt the number of open scopes).  We could also provide a nice CompositeResource implementes Resource base class for resources that have resources (i.e. tensor collections) that has a scope and protected attach methods, and implements close via that scope.

I'd also like to add a "weak" version of the resource counter scope that doesn't increment the count, just tries to free things (so if another scope uses and then discards them, they will be discarded then).  This would be useful in places like initScope or EagerSession where we don't want to keep things alive unnecessarily.

Would you still foresee the need to expose anything if we don't allow users to attach/detach stuff?

If we only ever assigned/attached to scopes on creation, we could get rid of these. We'd need a copyToNewScope like you suggested. That would essentially be reference counting though, so we might as well just go all the way (although that may require exposing the reference count methods, with the same issues). Another point for reference counting over copying is that I'd like the tensor scope to match the operand scope and copying operands is not easily doable. I'm gonna make another reply about reference counting in general.

If the copyToNewScope() refers to the creation of references and doing reference counting, yes, I think that's all we would need, although I think I'd prefer to call it more something like Tensor.newReference(newScope). I don't see the need to make anything else public or expose anything about reference counting either.

You mean because the weak refs will be dropped but the deallocation won't be done on the JavaCPP side? My thinking is to make the weakness optional, with it being strong by default if noPointerGC is true. Is that ever exposed anywhere?

There's that, but simply keeping weak references to pointers also doesn't work with GC anyway. The JVM needs at least 2 passes to clear weak and phantom references. We can (and will) lose access to weakly referenced pointers before we can get the chance to free them, deterministically, and then free the context they belong to, so memory will leak. We need to free the ops before the context gets freed. There is no way to enforce this ordering of events using GC. I think the original approach that was done for TF 1.x does work correctly tough, see #229 (comment):

Right, delete() and deleteAll() appear synchronized enough that calling them concurrently should be fine. Looking more closely, I think the only issue is when close() doesn't get called, the context itself never gets deleted. Anyway, if we wanted to do something similar to that with JavaCPP, I'd synchronize the deallocate() method here:
https://github.com/tensorflow/java/blob/master/tensorflow-core/tensorflow-core-api/src/main/java/org/tensorflow/internal/c_api/AbstractTFE_Op.java#L30
That gets assigned to Pointer.deallocator, which we can retrieve with the protected deallocator() method from those subclasses, keep strong references to them, and call deallocate() manually on them when desired. That should work well regardless of whether noPointerGC is enabled or not.

I'm still not entirely understanding this. You're saying that we may have a tensor get GC'd and then have the scope closed, where the pointer deallocate and the scope closing deallocate go off at the same time because the weak reference isn't removed in time? What are you referring to with "context"? There's nothing that needs to be freed other than tensors.

No, I'm saying that an object may get deallocated after the scope is closed. So, you will have live objects even after the scope is closed. You could argue that this isn't a big issue for tensors that have no context, but it renders the use of "scope" moot. If we can't even guarantee when everything is going to get deallocated we might as well have no scope at all! Moreover, in the case of tensors and ops that do have a context, it is a big deal:
// "Context" under which operations/functions are executed. It encapsulates
// things like the available devices, resource manager etc.
// TFE_Context must outlive all tensor handles created using it. In other
// words, TFE_DeleteContext() must be called after all tensor handles have
// been deleted (with TFE_DeleteTensorHandle).

https://github.com/tensorflow/tensorflow/blob/master/tensorflow/c/eager/c_api.h#L92

Implementation wise, if we want to generify it to Resource (which I think is a good idea), Resource would have to track and expose reference counters, which I don't like as it has all the same problems as exposing the scope. However, we could instead have scopes track their members and track all active scopes, using that to get the reference count instead. If we make the scope tracking weak, this also nicely removes references from GC'd scopes, although it won't necessarily clean up their now 0-reference resources (that would require another Cleaner like object and thread, but it's a backstop anyways so imo it's not worth that. Although if/when we add a Cleaner for something else we could use it here). Given that we shouldn't have a huge amount of scopes open, this should be fairly cheap (it's linear wrt the number of open scopes). We could also provide a nice CompositeResource implementes Resource base class for resources that have resources (i.e. tensor collections) that has a scope and protected attach methods, and implements close via that scope.

The only thing that needs reference counting in there are the underlying Pointer objects, and we can easily call retainReference() and releaseReference() on those for reference counting, without using PointerScope if that proves easier to manage. I don't see the need to expose any of that to the API. It could look like this and (probably) work correctly (not sure about all the details though):
try (Scope outerScope = Scope.create()) {
    Tensor t;
    try (EagerSession s = EagerSession.create()) {
      EagerOperation add =
          opBuilder(session, "Add", "ReturnResult")
              .addInput(tf.constant(2).asOutput())
              .addInput(tf.constant(4).asOutput())
              .build();
      t = add.tensor(0).newReference(outerScope);
    }
}

Precisely what do you feel would be missing here?

I'd also like to add a "weak" version of the resource counter scope that doesn't increment the count, just tries to free things (so if another scope uses and then discards them, they will be discarded then). This would be useful in places like initScope or EagerSession where we don't want to keep things alive unnecessarily.

You mean forced deallocation like a scope.deallocate()? Sure, why not, although it should be clearly documented as unsafe...

If the copyToNewScope() refers to the creation of references and doing reference counting, yes, I think that's all we would need, although I think I'd prefer to call it more something like Tensor.newReference(newScope). I don't see the need to make anything else public or expose anything about reference counting either.

It would work fine for just tensors, but doesn't work well for more generic Resources for the reasons I outlined in one of the last few comments (either creation would need to be handled in the scope, or attaching in the object, and it leaves resource counting to the user, which isn't an issue for Pointers but could be with other things).  Requiring all resources have a copy/clone style method is non-trivial.

No, I'm saying that an object may get deallocated after the scope is closed. So, you will have live objects even after the scope is closed. You could argue that this isn't a big issue for tensors that have no context, but it renders the use of "scope" moot. If we can't even guarantee when everything is going to get deallocated we might as well have no scope at all! Moreover, in the case of tensors and ops that do have a context, it is a big deal:
// "Context" under which operations/functions are executed. It encapsulates
// things like the available devices, resource manager etc.
// TFE_Context must outlive all tensor handles created using it. In other
// words, TFE_DeleteContext() must be called after all tensor handles have
// been deleted (with TFE_DeleteTensorHandle).

https://github.com/tensorflow/tensorflow/blob/master/tensorflow/c/eager/c_api.h#L92

You're saying that it's possible for a tensor to be marked weak (and removed from the set), but added to the deallotator reference key after the context, even though the context is still alive when the scope is closed?  So you're suggesting we synchronizing the deallocator and making it do nothing if the object is already deallocated, and then store the deallocator and call them before session/scope close (so the objects can still be GC'd, but we are guaranteed all deallocators fire).
That said, this is a non issue because the docs lie.  I went and tested it, and this:
val s = EagerSession.create()
val tf = Ops.create(s)
val a: Operand<TInt32> = tf.math.add(tf.constant(1), tf.constant(2))
TFE_DeleteContext(s.nativeHandle)
println(a.asTensor().getInt())

works fine, it even prints correctly.  Trying to create new ops after deletion does fail, but that's as intended.
In general, I think scopes are fine to not be strict about this.  You may have a resource close slightly after the scope, and we can note that in the docs, but requiring every resource to define a non-self-referencing deallocator is a lot of overhead when there aren't any situations that require it (eager context would be one if the above test didn't work).  Resources won't leak indefinitely, there would just be a slight delay (if I'm understanding things right).  If there is a need for strict resource -> context deallocation ordering it should be handled by the context.  @karllessard @Craigacp are you ok with this (specifically not being strict on ordering)?

The only thing that needs reference counting in there are the underlying Pointer objects, and we can easily call retainReference() and releaseReference() on those for reference counting, without using PointerScope if that proves easier to manage. I don't see the need to expose any of that to the API. It could look like this and (probably) work correctly (not sure about all the details though):
try (Scope outerScope = Scope.create()) {
    Tensor t;
    try (EagerSession s = EagerSession.create()) {
      EagerOperation add =
          opBuilder(session, "Add", "ReturnResult")
              .addInput(tf.constant(2).asOutput())
              .addInput(tf.constant(4).asOutput())
              .build();
      t = add.tensor(0).newReference(outerScope);
    }
}

Precisely what do you feel would be missing here?

This works fine if all resources use Pointer internally, but that's not necessarily always the case.  In particular, variables would be composite resources that hold onto a bunch of operands, and would have an internal scope to keep them alive, but would also themselves be Resources.  Functions would be similar, holding on to multiple ConcreteFunctions.

You mean forced deallocation like a scope.deallocate()? Sure, why not, although it should be clearly documented as unsafe...

Nah, I mean something that tries to close everything attached to it if a) it has no references and b) isn't already closed, but doesn't increment the count itself.  For cases like:

try(ResourceScope scope = new WeakResourceScope(){
  Tensor a = newTensor(scope);
  Tensor b = newTensor(scope);
  Composite c = new Composite(a, b);
  // stuff
  c.close()
}

where Composite does it's own scope management internally.  In cases like this we would want a and b to be closed when c is, and not outlive it because the outer scope has a reference.  And sure, in this example you could just close scope after c is created, but for longer lived scopes like init_scope that's not possible.

If the copyToNewScope() refers to the creation of references and doing reference counting, yes, I think that's all we would need, although I think I'd prefer to call it more something like Tensor.newReference(newScope). I don't see the need to make anything else public or expose anything about reference counting either.

It would work fine for just tensors, but doesn't work well for more generic Resources for the reasons I outlined in one of the last few comments (either creation would need to be handled in the scope, or attaching in the object, and it leaves resource counting to the user, which isn't an issue for Pointers but could be with other things). Requiring all resources have a copy/clone style method is non-trivial.

I'm not sure I see the issue, it feels to me like a pretty general way of doing things. It's just reference counting! It's what they do in the Python API. It works fine there, there's no reason we can't do everything the same way in Java. Maybe some things wouldn't be pretty, I don't know, but there's no reason it just wouldn't work. Would you have an concrete example of what you would like to do but that wouldn't work?

No, I'm saying that an object may get deallocated after the scope is closed. So, you will have live objects even after the scope is closed. You could argue that this isn't a big issue for tensors that have no context, but it renders the use of "scope" moot. If we can't even guarantee when everything is going to get deallocated we might as well have no scope at all! Moreover, in the case of tensors and ops that do have a context, it is a big deal:
// "Context" under which operations/functions are executed. It encapsulates
// things like the available devices, resource manager etc.
// TFE_Context must outlive all tensor handles created using it. In other
// words, TFE_DeleteContext() must be called after all tensor handles have
// been deleted (with TFE_DeleteTensorHandle).

https://github.com/tensorflow/tensorflow/blob/master/tensorflow/c/eager/c_api.h#L92

You're saying that it's possible for a tensor to be marked weak (and removed from the set), but added to the deallotator reference key after the context, even though the context is still alive when the scope is closed? So you're suggesting we synchronizing the deallocator and making it do nothing if the object is already deallocated, and then store the deallocator and call them before session/scope close (so the objects can still be GC'd, but we are guaranteed all deallocators fire).

Yes, that's what the code in TF 1.x was doing. Why do you guys keep pushing back against what was being done in TF 1.x? I mean, I know it's convoluted, but at least it kind of sort of worked and did what you want to do. (I still think it's a bad idea in general, so I'm not going to care personally about this.)

That said, this is a non issue because the docs lie. I went and tested it, and this:
val s = EagerSession.create()
val tf = Ops.create(s)
val a: Operand<TInt32> = tf.math.add(tf.constant(1), tf.constant(2))
TFE_DeleteContext(s.nativeHandle)
println(a.asTensor().getInt())

works fine, it even prints correctly. Trying to create new ops after deletion does fail, but that's as intended.
In general, I think scopes are fine to not be strict about this. You may have a resource close slightly after the scope, and we can note that in the docs, but requiring every resource to define a non-self-referencing deallocator is a lot of overhead when there aren't any situations that require it (eager context would be one if the above test didn't work). Resources won't leak indefinitely, there would just be a slight delay (if I'm understanding things right). If there is a need for strict resource -> context deallocation ordering it should be handled by the context. @karllessard @Craigacp are you ok with this (specifically not being strict on ordering)?

 And yes, resources can leak indefinitely, see, for example, https://blogs.oracle.com/javamagazine/epsilon-the-jdks-do-nothing-garbage-collector. That's the kind of thing that people using DJL are talking about. Like I already told @Craigacp, if that's not the kind of application that interests people here, they'll probably have to move away from TF Java and I'll help them get whatever they need with JavaCPP directly.

The only thing that needs reference counting in there are the underlying Pointer objects, and we can easily call retainReference() and releaseReference() on those for reference counting, without using PointerScope if that proves easier to manage. I don't see the need to expose any of that to the API. It could look like this and (probably) work correctly (not sure about all the details though):
try (Scope outerScope = Scope.create()) {
    Tensor t;
    try (EagerSession s = EagerSession.create()) {
      EagerOperation add =
          opBuilder(session, "Add", "ReturnResult")
              .addInput(tf.constant(2).asOutput())
              .addInput(tf.constant(4).asOutput())
              .build();
      t = add.tensor(0).newReference(outerScope);
    }
}

Precisely what do you feel would be missing here?

This works fine if all resources use Pointer internally, but that's not necessarily always the case. In particular, variables would be composite resources that hold onto a bunch of operands, and would have an internal scope to keep them alive, but would also themselves be Resources. Functions would be similar, holding on to multiple ConcreteFunctions.

Ok, so we should probably flesh this out a bit to figure out exactly what we want the thing to do. I don't see why having a detach() method alone would help with any of what you mention here.

You mean forced deallocation like a scope.deallocate()? Sure, why not, although it should be clearly documented as unsafe...

Nah, I mean something that tries to close everything attached to it if a) it has no references and b) isn't already closed, but doesn't increment the count itself. For cases like:
try(ResourceScope scope = new WeakResourceScope(){
  Tensor a = newTensor(scope);
  Tensor b = newTensor(scope);
  Composite c = new Composite(a, b);
  // stuff
  c.close()
}

where Composite does it's own scope management internally. In cases like this we would want a and b to be closed when c is, and not outlive it because the outer scope has a reference. And sure, in this example you could just close scope after c is created, but for longer lived scopes like init_scope that's not possible.

That sounds just like an implicit detach(). Panama isn't going in this direction (see https://github.com/openjdk/panama-foreign/pull/466), so I'm not sure this is the kind of thing we should explore, but sure, like I said, we're only doing alpha releases for now, so we can experiment. I'd be fine with that.

I'm not sure I see the issue, it feels to me like a pretty general way of doing things. It's just reference counting! It's what they do in the Python API. It works fine there, there's no reason we can't do everything the same way in Java. Maybe some things wouldn't be pretty, I don't know, but there's no reason it just wouldn't work. Would you have an concrete example of what you would like to do but that wouldn't work?

It would work, it's not impossible to do, but it requires a) every resource implementing a copy method, and b) the constructor and copy method must accept a scope and attach to the scope.  That's not something I want to force Resource classes to deal with when it can be handled as part of the scope.  To be clear, it's the implementation I have issues with, the general concept is fine.

 And yes, resources can leak indefinitely, see, for example, https://blogs.oracle.com/javamagazine/epsilon-the-jdks-do-nothing-garbage-collector. That's the kind of thing that people using DJL are talking about. Like I already told @Craigacp, if that's not the kind of application that interests people here, they'll probably have to move away from TF Java and I'll help them get whatever they need with JavaCPP directly.

If you're using a "Do nothing GC" but still relying on JavaCPP's GC cleanup, I don't think that's our issue.  This would work fine with Epsilon as long as JavaCPP's noPointerGC is true (since they would be strong references).  We can separate the strong references from noPointerGC a bit and provide an option to always use strong references in scopes that defaults to noPointerGC, which covers even the edge cases.
We could also make registering a deallocator optional, and use it for Pointer based classes, I just don't want to require it for every resource when it's almost always a non issue and is a decent amount of overhead to implement (and prone to confusing and hard to debug bugs if you keep the resource reference around).

Ok, so we should probably flesh this out a bit to figure out exactly what we want the thing to do. I don't see why having a detach() method alone would help with any of what you mention here.

I'm not proposing a detach method.  This has nothing to do with detach.  I'm proposing a fully reference counting scope instead of the only tracking version there currently, that hides the reference counting in internal PointerScopes.  It wouldn't have a detach method (although it may want something like releaseReference, we'll see).

That sounds just like an implicit detach(). Panama isn't going in this direction (see openjdk/panama-foreign#466), so I'm not sure this is the kind of thing we should explore, but sure, like I said, we're only doing alpha releases for now, so we can experiment. I'd be fine with that.

Again, detach doesn't have anything to do with this.  This is analogous to the default scope mentioned in that PR, but instead of a single default it's one that can be used by our existing top level scopes.  It still "owns" resources in the sense that it will close them when it closes, but doesn't prevent them being closed earlier if they are part of some other earlier-closing scope.

As per the above comment, I'm closing this PR, we need a non-tensor specific resource scope.  I don't have time to do it at the moment, but may eventually.


Keep TestCafe running for rapid development

Are you requesting a feature or reporting a bug?
Feature request.
What is the current/expected behavior?
After each testrun, TestCafe stops and closes the browser window(s).
During the development of the integration tests, the startup and teardown of TestCafe is very time consuming.
It would be nice to have TestCafe remain open/active and watching for changes in the test files. If a change is made, the current running tests would abort and the updated test files loaded and executed. All this without TestCafe doing it's browser resize dance and bootstrapping :).
If such behavior is already possible, please direct me to the documentation :).

I've been evaluating Cypress alongside Testcafe and watch mode is something that they really do right. It's so fast it makes writing E2E tests feel as good as writing unit tests.

Hi, thanks for your feedback. I believe the watch mode is a very useful thing. I guess we'll provide some news about this the next week.

https://github.com/DevExpress/testcafe-live is broken

Hi @xcb6,

testcafe-live is broken

Could you please clarify what do you mean? You can't install it or can't run tests?
I call the following command on my machine and it runs tests succesfully:
npm install testcafe testcafe-live -g
testcafe-live chrome d:\tests\example\example.js

https://github.com/DevExpress/testcafe-live => 404

oops. you're right. Fixed

feedback (npm install) => very very great !!!! it's ok on win7

Hi everyone who is interested in this feature. We've done some work and published the testcafe-live module. The full description about it you can read in the TestCafe Live repository.
It would be greatly appreciated if you try it and provide us with your feedback!
/cc @jvanoostveen @chandureddivari @kartojal, @tamlyn @jonataswalker @gmschouten @fiws @noderat @RickHoving @FabioAntunes @Dattaya @tijhaart and others

\cc @ganlanyuan @ervoiox @chinjon @ablipan @pierreh @xcb6

We've done this in TestCafe Live so I close the issue. I guess we'll add info about it to the Readme soon.


SQL Import Structure Issue

I can't figure this out.  I am a bit of a noob but i think this is the last piece to my puzzle.
I am importing per the instructions on the core documentation utf8mb4_unicode_520_ci and 17 tables import.
This is the query i am getting the error on.  I feel like if i fix this....i'll have my first telebot working!
any help would be apprciated.  I have tried to import this query with different tags and stuff but have had no luck.  Would love some help!
MySQL said: Documentation
#1267 - Illegal mix of collations (utf8mb4_general_ci,COERCIBLE) and (utf8mb4_unicode_520_ci,COERCIBLE) for operation '<>'
The Query:
( SELECT User, Host, Select_priv, Insert_priv, Update_priv, Delete_priv, Create_priv, Drop_priv, Grant_priv, Index_priv, Alter_priv, References_priv, Create_tmp_table_priv, Lock_tables_priv, Create_view_priv, Show_view_priv, Create_routine_priv, Alter_routine_priv, Execute_priv, Event_priv, Trigger_priv, '*' AS Db, 'g' AS Type FROM mysql.user WHERE NOT (Select_priv = 'N' AND Insert_priv = 'N' AND Update_priv = 'N' AND Delete_priv = 'N' AND Create_priv = 'N' AND Drop_priv = 'N' AND Grant_priv = 'N' AND References_priv = 'N' AND Create_tmp_table_priv = 'N' AND Lock_tables_priv = 'N' AND Create_view_priv = 'N' AND Show_view_priv = 'N' AND Create_routine_priv = 'N' AND Alter_routine_priv = 'N' AND Execute_priv = 'N' AND Event_priv = 'N' AND Trigger_priv = 'N')) UNION ( SELECT User, Host, Select_priv, Insert_priv, Update_priv, Delete_priv, Create_priv, Drop_priv, Grant_priv, Index_priv, Alter_priv, References_priv, Create_tmp_table_priv, Lock_tables_priv, Create_view_priv, Show_view_priv, Create_routine_priv, Alter_routine_priv, Execute_priv, Event_priv, Trigger_priv, Db, 'd' AS Type FROM mysql.db WHERE 'telebot4db' LIKE Db     AND NOT (Select_priv = 'N' AND Insert_priv = 'N' AND Update_priv = 'N' AND Delete_priv = 'N' AND Create_priv = 'N' AND Drop_priv = 'N' AND Grant_priv = 'N' AND References_priv = 'N' AND Create_tmp_table_priv = 'N' AND Lock_tables_priv = 'N' AND Create_view_priv = 'N' AND Show_view_priv = 'N' AND Create_routine_priv = 'N' AND Alter_routine_priv = 'N' AND Execute_priv = 'N' AND Event_priv = 'N' AND Trigger_priv = 'N')) ORDER BY User ASC, Host ASC, Db ASC;

The error from trying to run getUpdatesCLI.php is below.
PHP Fatal error:  Uncaught TypeError: Argument 2 passed to Longman\TelegramBot\DB::insertTelegramUpdate() must be of the type int or null, float given, called in /var/www/html/vendor/longman/telegram-bot/src/DB.php on line 620 and defined in /var/www/html/vendor/longman/telegram-bot/src/DB.php:338
Stack trace:
#0 /var/www/html/vendor/longman/telegram-bot/src/DB.php(620): Longman\TelegramBot\DB::insertTelegramUpdate()
#1 /var/www/html/vendor/longman/telegram-bot/src/Telegram.php(644): Longman\TelegramBot\DB::insertRequest()
#2 /var/www/html/vendor/longman/telegram-bot/src/Telegram.php(516): Longman\TelegramBot\Telegram->processUpdate()
#3 /var/www/html/getUpdatesCLI.php(27): Longman\TelegramBot\Telegram->handleGetUpdates()
#4 {main}

this is resolved.  can't be on 32 bit raspberry OS.

i was on 32 bit os


New project ideas for PolyPhy

Two new project ideas:

PolyPhy Web Presence
Data Visualization and Analysis with PolyPhy/Polyglot

@slieggi @carlosmalt


Add basic support for window icons

Objective
Fixes https://github.com/bevyengine/bevy/issues/1031.
This PR adds an icon to the left of the title in the window taskbar on Windows and X11.
Looking for high-level feedback on whether this implementation is going in the right direction before cleaning up and submitting.  A lot of the concepts in Bevy are new to me, and it sounds like the approach that has been suggested is a workaround and might not have a lot of prior art to consult.  More context here.

Solution

TBD

Changelog
Add an icon to the left of the title in the window taskbar on Windows and X11.
Migration Guide
N/A

Okay good! The overall module structure like this is much less bad than I was worried about. I think we can make the underlying system a bit less janky, but once that's done this should work nicely.

Does it make sense to provide an API for setting the window icon? On Linux, you're supposed to use a .desktop file (which also lets it show up in UI's). On Windows, you're supposed to embed a resource file into the exe. On macOS I'm not sure.

Does it make sense to provide an API for setting the window icon? On Linux, you're supposed to use a .desktop file (which also lets it show up in UI's). On Windows, you're supposed to embed a resource file into the exe. On macOS I'm not sure.

I like the cross-platform nature of this: it'll make the build process significantly less annoying when releasing games.

Does it make sense to provide an API for setting the window icon? On Linux, you're supposed to use a .desktop file (which also lets it show up in UI's). On Windows, you're supposed to embed a resource file into the exe. On macOS I'm not sure.

macOS, Android and iOS are also handled by providing a file with the binary

making bevy_render depends (optionally but enabled by default) doesn't feel good for something that won't work on most platforms

Looking into why the build is failing for wayland-sys.  Someone might what's going on here.

Looking into why the build is failing for wayland-sys. Someone might what's going on here.

you enabled the wayland feature of bevy_winit but wayland dependencies are not installed in CI

you enabled the wayland feature of bevy_winit but wayland dependencies are not installed in CI. You change makes it always enabled.

I probably misunderstood this request, in that case.

Is this approach in this PR basically sound?  Should I clean it up and move it out of draft?  Or should I abandon it and let someone else make an attempt at it?  I don't have any problem abandoning it.

how is progress looking?

how is progress looking?

Hi @Adrian8115.  This PR is not active.  I should close it.

Is there a pr that allows us to set the icon of a winow easily? This is a chain of prs that all link to each other but none have been successful

Is there a pr that allows us to set the icon of a winow easily? This is a chain of prs that all link to each other but none have been successful

After this PR, I stopped keeping tabs on Bevvy.  I do not know what they're currently up to, unfortunately.

Is there a pr that allows us to set the icon of a winow easily? This is a chain of prs that all link to each other but none have been successful

The correct solution imo is to package an icon with your compiled app.

On windows, embed an ico within the exe
On linux, ship a .desktop file and png/svg with your app
On macOS, I know it's possible, but I'm not familiar with it.


Repo sync for protected CLA branch

The pull request is created from main637746711000111336 to main to fix git push error for protected CLA branch

@opbld15 : Thanks for your contribution! The author(s) have been notified to review your proposed change.


Feature/workflow tweaks

Changes proposed in this Pull Request:

EULA is now the first screen in the workflow.
Added optioned to include first/last name regex input requirements - added docs.

There's one additional item on JIRA to remove message from the login screen whenever a new character is entered, but I think this logic already existed on the angular side.

They custom error message for the "Last Name" field isn't showing up.

I like that you can add any regex and add multiple requirements, but it would be nice to make use of the mat-input "maxLength" input for this.

Can you change the "Back" button text to "Cancel" for the first step of both registration workflows? This was updated for react and react native already. We had bugs written for those frameworks and must have missed writing up the Angular ones.

The custom error message for the "Last Name" field isn't showing up.

are we able to catch regex as we are typing to trigger the limit? My error didn't trigger until I tabbed out of the field. If I edit the field to be less than 30 char the error state still exists

I didn't see any changes for clearing the invalid credentials error message on the login screen after edits are made — was that already working as expected?

I think this was already working as expected, I'm not sure what this feature request was referring to because if I'm on the login page and I enter an invalid email, the error message disappears on edit.  No work required for this, this is how it was.

I like that you can add any regex and add multiple requirements, but it would be nice to make use of the mat-input "maxLength" input for this. This would result in the end-user not being able to enter more characters than they are allowed. This would also be more in sync with our design patterns for this kind of validation.

I'm going to try to match the RN implementation and just remove the regex for now.  If we need more advanced customization, then we can re-introduce it in the future.  For now, probably best to keep it simple.

are we able to catch regex as we are typing to trigger the limit? My error didn't trigger until I tabbed out of the field. If I edit the field to be less than 30 char the error state still exists.
Also if first / last name has a white space we trigger char count exceeded

Now the implementation will stop a user from typing once they exceed the character limit.

This should be good to go now; one additional change that was made after today's demo was that the default maxLength was increased from 30 characters to 1000 characters.
The [maxLength] directive that angular provides only accepts a number, so I cannot pass in undefined because it'll interpret that as 0.  The default value of 1000 characters is supposed to be the same as 'no character limit'.


⚠ ClamAV has degraded performance

In 4262ab8, ClamAV (https://spamassassin.apache.org/) experienced degraded performance:

HTTP code: 200
Response time: 295 ms

Resolved: ClamAV performance has improved in 13517d6.


Update README.md to include arch and arch/kernel directories

The new arch directory is not yet documented in README.md.

c1e1992e281cd0e44354dae3ffcd00de3ee67655 adds the kernel/arch directory, which should also be listed.

Marked as low priority because there might be more structural changes soon.

More changes in d35d449177c1246486b83d7b7bf54f006cdbb38f

i will do this now see my pul request

Instead, I decided to remove the Organization section from the readme. The docs should contain a fixed version of the Organization somewhere.


Get rid of the module-manager

Description
Changes proposed in this pull request:

...
...
...

Related issue(s)

/test all


typing issue?

I think there might an issue with the typings (Typescript > 2.4.x ) - do correct me if I'm wrong.
But when I return a ES6Promise when a native Promise is expected - I get a TS compiler error saying
...
"Property '[Symbol.toStringTag]' is missing in type 'Promise'."
...
There is more to the error, but I'm just pasting the last line. So, here I'm returning a ES6Promise when a Promise is expected.

It seems like the typings expecting Symbol.toStringTag are too strict. Rather they should merely expect something with then is returned. Promises are designed to inter-opt entirely on the existing of  then function member.


It is challenging to find the list of checks/tests in the docs

Observed behaviour
I have been chatting with a couple of font designers/developers about FontBakery recently, and I went to find where in the docs the current checks were listed. I eventually found the list of profiles:
https://font-bakery.readthedocs.io/en/latest/fontbakery/profiles/index.html
Expected behaviour
It's probably a fairly common question for people researching the project for the first time:

What does this test for?

So, I would expect a link to the full set of tests available to be highlighted more prominently on the front page of the docs.
Also, maybe there could be a more obvious name to get there than "profiles"? That title requires some familiarity with how FontBakery is set up to even search for. Possibly, the top-level navigation could be something like "Tests" or "Checks" or possible "Test Profiles"? Not sure what the best answer is, but if we can think of what a newcomer might be looking for, it would probably be helpful.

The README on this git repo has this:
"For a quick overview, check out the list of checks currently offered."
with a link to https://font-bakery.readthedocs.io/en/latest/fontbakery/profiles/index.html
Maybe we should add the same sentence to the beginning of the documentation homepage at https://font-bakery.readthedocs.io/en/latest/


Flat directory for all artefacts

All artefacts are being cached in the same folder. This causes problems when artefacts with different group ids share the same artefact id. When the versions collide then any new to be uploaded artefact will use the previously downloaded one. Solution will be to uniquely identify all artefacts.
### Tasks
- [ ] https://github.com/microsoft/mvnfeed-cli/pull/23

Fixed in PR#23


Make suitcase-utils noarch

Tested via https://github.com/nsls-ii-forge/suitcase-utils-feedstock/pull/1.

Thank you, @tacaswell!


Add vercel and nextjs descriptions to README

I guess the main purpose of this action is to get the deployment URL of a nextjs installation deployed on Vercel. Might be interesting to write some additional documentation for that. Might also be interesting to reference the blogpost you've written about it :)

Actually it is used with a Gatsby site on Vercel, but the principal is the same. I'm still planning on writing a more elaborate post about integrating GitHub actions with the GitHub API, so I'll link to it when it's done.

Ahh, I'm using it with NextJS, so it's really more about Vercel :)


Update NuGet-related sections in csproj for nuget.org release

This is based on the documentation here:
https://docs.microsoft.com/en-us/nuget/schema/msbuild-targets#pack-target


WindowDataset will not be optimized

I have pushed some code to the keras branch. It is sort of working, but I am getting this warning
Input of WindowDataset will not be optimized because the dataset does not implement the AsGraphDefInternal() method needed to apply optimizations

This is definitely not a good thing. Initial googling did not lead to an obvious improvement. Ideas?

this was fixed!


Invalid links in docs

The doc for data fetching (https://github.com/apollographql/graphql-tools/blob/84e77ac7d7e06f149019c5a42dd2e86907e27ac1/docs/source/connectors.md) is referencing to the repo apollostack/GitHunt-API which doesn't exist anymore.

Rolling into #1316


Responsive design broken in landscape mode

iPhone SE portrait mode (works)

iPhone SE landscape mode (broken)

what are we supposed to show? mobile design taking 100% of the width and centring everything? or maybe disallow landscape temporarily until we have landscape designs?

I suggest simply changing the breakpoints to the smallest possible value which is wide enough for the desktop design to fit. (I mean, the booking + the help bar.)

I would suggest to remove also all (min-width: 320px) - it shouldn't we necessary, just saying "everything up to this value" is fine

Agreed... Even if the screen is so small that the design breaks, it should break less than the desktop version :-D


Documentation Section

We need to create a proper section for documenting how the application works to let possible contributors help with development.
It is also helpful to better understand what the specific goal of this project is and how to achieve it.

The section has created but it needs to be expanded (a lot).


hidden feature

hello,i input an image with shape(3,480,480), i want to vision the hidden feature (240,240).
there is an error about numpy's broadcast .
cam += w.cpu().data.numpy() * feature_map[i, :, :].cpu().data.numpy()
ValueError: operands could not be broadcast together with shapes (480,480) (240,240)

Could you provide more details about your code?


[TwigComponent] Html syntax bug test case + fix

Q
A

Bug fix?
yes

New feature?
no

Tickets
Fix #805

License
MIT

Not a fix, just a test case for the bug described in #805. It's probably easy to fix.
This fails to add the correct content block inside of twig:foo:
<twig:foo>
    <twig:bar>bar content</twig:bar>
</twig:foo>

But if you add any non-whitespace character before <twig:bar> - e.g. foo <twig:bar> - then it WILL add the block correctly. So, just some small thing not looking for an embedded component as valid "content" that should open that content block.

Now this also has the fix :)


Update embedded-kafka to 3.0.0

Updates io.github.embeddedkafka:embedded-kafka from 2.8.1 to 3.0.0.
GitHub Release Notes - Version Diff
I'll automatically update this PR to resolve conflicts as long as you don't change it yourself.
If you'd like to skip this version, you can just close this PR. If you have any feedback, just mention me in the comments below.
Configure Scala Steward for your repository with a .scala-steward.conf file.
Have a fantastic day writing Scala!

Ignore future updates
Add this to your .scala-steward.conf file to ignore future updates of this dependency:
updates.ignore = [ { groupId = "io.github.embeddedkafka", artifactId = "embedded-kafka" } ]

labels: test-library-update, early-semver-major, semver-spec-major, commit-count:1

Superseded by #220.


chore(CPlusPlus): add print middle element

PR Checklist:

[X ] My submission is formatted according to the guidelines in the contributing guide
[ X] My addition is on refer on the language README.md file
[ X] My addition does not have a spelling problem
[X ] My submission has a proper and user-friendly description of the algorithm
[ X] My submission has the time complexity of the algorithm
[X ] My submission has sample input-output of the program (NOT FOR PYTHON)

What kind of change does this PR introduce? (check at least one)

[ ] Bugfix
[X ] New algorithm
[ ] Optimization in previous algorithms
[ ] Code style update
[ ] Refactor
[ ] Documentation
[ ] Other, please describe:

Briefly describe the changes in this PR
Given a singly linked list, find the middle of the linked list. For example, if the given linked list is 1->2->3->4->5 then the output should be 3.
If there are even nodes, then there would be two middle nodes, we need to print the second middle element. For example, if the given linked list is 1->2->3->4->5->6 then the output should be 4.
Other information:

@creativeyashi don't remove the PR checklist. Make sure you keep it and check all the relevant boxes, that way you and i will know what is missing.

@creativeyashi don't remove the PR checklist. Make sure you keep it and check all the relevant boxes, that way you and i will know what is missing.

Sure,But for now please merge it.or I need to make another PR for the same.

Done with changes.


Renaming a Web Application

Details of the scenario you try and problem that is occurring:
FEATURE REQUEST:
This ask came up after a session I delivered in Ottawa on SPDSC. Is there a way to make the SPWebApplication resource handle the scenario where we are trying to rename a Web Application? Maybe by providing an optional GUID parameter and detecting if the name in the config is different from the one currently assigned to a Web App with that GUID?
The DSC configuration that is using the resource:
N/A
Version of the Operating System and PowerShell the DSC Target Node is running:
N/A
Version of the DSC module you're using:
<IP_ADDRESS>

Hi Nick. I was at your session and have been thinking about this.
I'm not sure a rename belongs in any DSC, because it is an operation, or a "way of achieving the desired state", not the actual desired state itself.
What do you think?
Thanks,
Chris

There may be cases however that are more of an "update" scenario than a "set". For example, if you have a service instance that is enabled, and then push a new dsc that mentions it as disabled, you need to perform the operation to shut it down. But I agree 100% with what you are saying. I believe that scenario falls right in the grey zone. I am not convinced we'll want to tackle this as part of the spwebapplication resource but wanted to have it logged.

Yea this is a tricky one for us. Essentially what it boils down to is that we need a consistent "identifier" for each instance of each resource. In terms of the web app resource we decided the display name would be it since it was more likely the URLs would change than the display name property (based on it representing the intent of the web app vs a URL which might need to be updated). So updating the primary key kind of breaks the model in that its no longer representing that instance of a resource but a new one. Now we could have a chat about whether the URL or the display name should be the primary key (which would be a breaking change to implement) but I tend to agree with the approach we took here, so based on that I don't think this is an issue for DSC to solve. I think its something to be done manually or through a custom script and then update the DSC to reflect the new desired state.
@ykuijs you got any thoughts on this one?

I thought about this scenario before, and think it's just as easy to dismount content DBs, delete the web app, and push a new config with the new name and desired settings, and remount the content DBs.

@MikeLacher448 I don't think the challenge lies in the logic of making it happen as much as it lies in what other implications this would have. We picked the Web Application's Name as our primary key. In this scenario we are trying to update the primary key of an instance. I am just not sure we want to open this can of worms.

@NikCharlebois and in lieu of any objection from @ykuijs I tend to agree, so I'm gonna close this one as a "won't fix" and leave it to be handled outside of the realm of DSC. Happy to re-open if anyone has a different perspective that will bring me around though.

Sorry guys, little busy the last few days :-)
Totally agree with these conclusions. Don't think this is a scenario which we should solve using DSC


Undefined is not an object (evaluating 'item.props.cluster')

This code was working with react-native-maps MapView. It shows an error with this package.
 <MapView
               initialRegion={this.state}
                clusterColor = '#000'
                clusterTextColor = '#fff'
                clusterBorderColor = '#fff'
                clusterBorderWidth = {4}
                ref = {(ref)=>this.mapView=ref}
                onClusterPress={(coordinate)=>{
                    this.animate(coordinate);
                }}
                radius={25}
                style={styles.map}
                region={region}
                showsUserLocation = {true}
                 >
               {  this.renderMarker(selectedExplore)}
                        <WardBorder/>
                </MapView>
And on WardBorder component
<Polygon
                    coordinates={cords}
                    strokeColor="rgba(0,0,0,0.2)"
                    fillColor="rgba(0,0,0,0.1)"
                    strokeWidth={1} />
But if i comment out the WardBorder (polygon) component marker is clustered.

Make following changes in react-native-map-clustering/MapView/MapWithClustering.js file
       if (item.props && item.props.cluster !== undefined) {
          canBeClustered = item.props.cluster
        }
        // item.props.cluster === undefined ? canBeClustered = true : canBeClustered = item.props.cluster;

I released new version of this library. This issue should be solved 🎉


Update package.json

Coverage report

St.:grey_question:
Category
Percentage
Covered / Total

🟢
Statements
100%
61/61

🟢
Branches
100%
11/11

🟢
Functions
100%
1/1

🟢
Lines
100%
61/61

Test suite run success
6 tests passing in 1 suite.
Report generated by 🧪jest coverage report action from 55dfa73691a6a3ce417b594420d41dd3176e7d05


#251 - Send email to subscribers.

I have a problem with $.notify function - it doesn't seem to work - I always get the following error:
Uncaught TypeError: $.notify is not a function


Windows: Fix filesystem subscription impl tests

Commit Message:
Windows: Fix filesystem subscription impl tests
Replace dispatcher and watcher with mocks and orchestrate the test
harness to capture filesystem subscription impl file event callback and
invoke it directly. Test no longer relies on watching real filesystem
events and avoids timing discrepancies causing flakiness.
Additional Description: N/A
Risk Level: Low
Testing: Modifies unit tests, tested locally on Windows
Docs Changes: N/A
Release Notes: N/A

cc @envoyproxy/windows-dev

Agreed that MemFileSystem would be larger than this change, but it would also have a lot more beneficial impact, for Windows in general but also across the testsuite, making tests faster and less flaky.
I suspect that the existing test actually covers something that this now mocks out, and I'm not sure if changing this would drop the coverage of it. But maybe someone who's more involved in the filesystem watcher can comment.

@envoyproxy/maintainers seeking help on reviewing this from someone who knows more about the filesystem watcher.

I suspect that the existing test actually covers something that this now mocks out

Yes, this test is written expecting synchronous callback execution. In the real world, the NTFS kernel layer is not behaving synchronously. Solving this test design flaw requires a lot more code to anticipate watch notifications arriving asynch, and that isn't what this test is designed to exercise. We are simply exercising that a notification will cause a refresh of the config.
This seems better solved with the simplified mock than trying to anticipate the real-world execution of this test.

Seems like it should be possible to leave the test structure as is but allow for async vs sync notificaton; not even sure what that means, to be honest.
Shouldn't the intervening library synchronize the async notification so that callers can be expect the same behavior whether running on Windows or Linux or Mac?
If we have to be changing the calling code to one of our libraries to add ifdefs for subtle behavioral differences, we are going to have a bad time.

Shouldn't the intervening library synchronize the async notification so that callers can be expect the same behavior whether running on Windows or Linux or Mac?

We are removing all async code from this test using this method of dependency injection. The class under test doesn't inherently need to react to asynchronous events. The watcher implementation is tested elsewhere: https://github.com/envoyproxy/envoy/blob/master/test/common/filesystem/watcher_impl_test.cc
We could continue to test the FilesystemSubscriptionImpl class with the dispatcher/libevent event loop and filesystem watcher with added synchronization that did not exist prior, however it seems simpler to remove any need for blocking or cross-thread synchronization etc. What is the approach the core maintainers want us to follow? @envoyproxy/senior-maintainers do you have any thoughts or direction for us?

If we have to be changing the calling code to one of our libraries to add ifdefs for subtle behavioral differences, we are going to have a bad time.

We shouldn't be adding any ifdefs for this class of issues, we are in fact trying to factor tests such that they are more correct across all platforms. Envoy relies on asynchronous event loops and callbacks and a number of tests make assumptions about timing and sequencing of events which are not definitive. We are trying to refactor tests so they eliminate assumptions or assertions are flexible enough to react correctly to these indefinite conditions.
Separate issue, which might be confusing things:
The different filesystem watcher implementations are all watching file events asynchronously by definition, when watching the local filesystem, our tests have relied on the fact that Linux+ext4 behaves synchronously/almost instantly. If we were to run the tests on Linux backed by NFS or other filesystems, e.g in GCP or other CI hosts, we may see similar filesystem event timing issues as we see on Windows. This design defect is commonly observed in implementations of locking strategies when tested only using one process on the local filesystem. Our recent fixes to the filesystem watcher tests patiently wait for asynchronous notices to fire (see: https://github.com/envoyproxy/envoy/pull/12496).

Seems like an unrelated coverage test failure, merged master to hopefully re-kick with success

Yeah, I agree that a memory/fake filesystem will be useful for a bunch of tests. In this specific case, for FilesystemSubscriptionImpl, I'm fine with the change as is. @sunjayBhatia do you anticipate having to do this for a bunch of other tests? This is probably the key weighing factor.

@htuch at this point, following Matt's latest fix, some 27 integration tests and some 14 other tests are still to be resolved. As we discover more patterns, we can fix, and also go back over previously fixed tests which exhibited the same pattern. A memory filesystem would be a great addition, but I don't think it holds up merging this specific fix.

 In this specific case, for FilesystemSubscriptionImpl, I'm fine with the change as is. @sunjayBhatia do you anticipate having to do this for a bunch of other tests? This is probably the key weighing factor.

It seems like the dispatcher etc. is appropriately mocked in other tests etc. so I would guess we would not have to do this same exercise again (also b/c I think this is the lowest level abstraction on top of the filesystem watcher which shouldn't have too much more involvement).


fix(permission): fix CheckLoginPermission() logic

fix issue: #2388

 Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.aidenlu seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.You have signed the CLA already but the status is still pending? Let us recheck it.


WIP: Docs refactor

@bbartley These are some changes I wanted to make, but I hit a block running the pytests because of the version deps.  It does however improve the spec generation code a bit.

Yeah, you'll likely want to leave the dependencies for PAML alone this week: setup.py has been pinned with the old versions for a while because there's a bunch of pain associated with the rdflib 5 to rdflib 6 transition. @bbartley started the upgrade to bring PAML across the change as well, but I'm a bit nervous about poking it just before demos.