Multiple errors thrown on reaching max retries by #580

NOTE: My first issue to a public repo, any feedback is appreciated :)
Describe the bug
Using spotipy post-#580 (relevant issue #571 ) yields uncaught urllib3 / requests library errors, and in particular an AttributeError due to retry_error.response being None (see below). Furthermore, though not featured below, line 274 of client.py under this commit will also throw an AttributeError for the same reason.
Your code
Note: Have defined environment variables SPOTIPY_CLIENT_ID, SPOTIPY_CLIENT_SECRET, SPOTIPY_CLIENT_USERNAME and SPOTIPY_REDIRECT_URI.
# GetClientCredentials() also works here, the playlist is public
spotify = Spotify(auth_manager=SpotifyOAuth(scope='playlist-read-private'))
brain_food_playlist_id = '37i9dQZF1DWXLeA8Omikj7'
playlist = spotify.playlist(playlist_id=brain_food_playlist_id)
print(spotify.playlist_is_following(playlist_id=playlist['id'], user_ids=['spotify:user:bad_username']))

Expected behavior
According to the implementation of the requests library, responses are expected to not be passed to RetryErrors (see source code here; AFAIK this is the only circumstance it is thrown by the library).
Therefore, it is my belief that:

A SpotifyException should be thrown (potentially, with the associated url and header, if retrievable)
(if would not unreasonably impact existing behaviour) The non-spotipy exceptions should not be thrown

Output
WARNING: Paths modified for privacy
Max Retries reached
Traceback (most recent call last):
  File "../project/venv/lib/python3.8/site-packages/requests/adapters.py", line 439, in send
    resp = conn.urlopen(
  File "../project/venv/lib/python3.8/site-packages/urllib3/connectionpool.py", line 817, in urlopen
    return self.urlopen(
  File "../project/venv/lib/python3.8/site-packages/urllib3/connectionpool.py", line 817, in urlopen
    return self.urlopen(
  File "../project/venv/lib/python3.8/site-packages/urllib3/connectionpool.py", line 817, in urlopen
    return self.urlopen(
  File "../project/venv/lib/python3.8/site-packages/urllib3/connectionpool.py", line 807, in urlopen
    retries = retries.increment(method, url, response=response, _pool=self)
  File "../project/venv/lib/python3.8/site-packages/urllib3/util/retry.py", line 439, in increment
    raise MaxRetryError(_pool, url, error or ResponseError(cause))
urllib3.exceptions.MaxRetryError: HTTPSConnectionPool(host='api.spotify.com', port=443): Max retries exceeded with url: /v1/playlists/37i9dQZF1DWXLeA8Omikj7/followers/contains?ids=spotify:user:bad_fakename (Caused by ResponseError('too many 500 error responses'))

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "../project/venv/lib/python3.8/site-packages/spotipy/client.py", line 239, in _internal_call
    response = self._session.request(
  File "../project/venv/lib/python3.8/site-packages/requests/sessions.py", line 530, in request
    resp = self.send(prep, **send_kwargs)
  File "../project/venv/lib/python3.8/site-packages/requests/sessions.py", line 643, in send
    r = adapter.send(request, **kwargs)
  File "../project/venv/lib/python3.8/site-packages/requests/adapters.py", line 507, in send
    raise RetryError(e, request=request)
requests.exceptions.RetryError: HTTPSConnectionPool(host='api.spotify.com', port=443): Max retries exceeded with url: /v1/playlists/37i9dQZF1DWXLeA8Omikj7/followers/contains?ids=spotify:user:bad_fakename (Caused by ResponseError('too many 500 error responses'))

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "../project/src/issue_boilerplate.py", line 10, in <module>
    print(spotify.playlist_is_following(playlist_id=playlist['id'], user_ids=['spotify:user:bad_fakename']))
  File "../project/venv/lib/python3.8/site-packages/spotipy/client.py", line 1145, in playlist_is_following
    return self._get(
  File "../project/venv/lib/python3.8/site-packages/spotipy/client.py", line 286, in _get
    return self._internal_call("GET", url, payload, kwargs)
  File "../project/venv/lib/python3.8/site-packages/spotipy/client.py", line 273, in _internal_call
    "%s:\n %s" % (response.url, "Max Retries"),
AttributeError: 'NoneType' object has no attribute 'url'

Environment:

OS: Ubuntu 20.04.1 LTS
Python version: 3.8.2
spotipy version: Commit 18e82b6625ca1102c12da08194c806f0fc78f56a (#580 )
your IDE: xterm

Hi @Quizz1Cal, thanks for the nice report.
In your opinion would reverting https://github.com/plamere/spotipy/pull/580 and re-opening https://github.com/plamere/spotipy/issues/571 help? Also if you feel confident to write a fix for this, that would be really appreciated  🙏

In the sense that #580 does not spiritually resolve #571, perhaps a revert would be best.
In terms of a fix, I'm basically already there so going the full distance should be fine! It comes down to what you'd want to see in a thrown SpotifyException. I can extract the true url from https://api.spotify.com/{v1/etc...} from the error.request.url, which I presume would always equal to response.url. Shouldn't be hard to catch the other exceptions, but can you confirm it would be appropriate to catch or otherwise avoid throwing the urllib3 / requests library errors?
If you approve the PR, would you be willing to label it has hacktoberfest-accepted? (It's not the reason I'm using spotipy, so I'll still fix it regardless :) )

I've developed a fix:
# ... spotipy/client.py line 267
except requests.exceptions.RetryError as retry_error:
    request = retry_error.request
    logger.error('Max Retries reached')
    try:
        reason = retry_error.args[0].reason
    except (IndexError, AttributeError):
        reason = None
    raise SpotifyException(
        599,
        -1,
        "%s:\n %s" % (request.path_url, "Max Retries"),
        reason=reason
    )

I have only attempted action (1) from above; it should be noted that catching the SpotifyException itself here will silence all aforementioned errors.
I'm not sure if I should open a PR if you haven't yet reverted #580 , so I figure I'd post the minor change here for now. It passes all tests, though I don't believe we have any tests to deal with this specific issue. Perhaps something to add?

In the sense that #580 does not spiritually resolve #571, perhaps a revert would be best.

I reverted https://github.com/plamere/spotipy/pull/580 in https://github.com/plamere/spotipy/pull/582 and reopened the initial issue https://github.com/plamere/spotipy/issues/571. In your PR, feel free to solve both #571 and #581

If you approve the PR, would you be willing to label it as hacktoberfest-accepted?

Definitely!

can you confirm it would be appropriate to catch or otherwise avoid throwing the urllib3 / requests library errors

Your fix looks good, displaying retry_error.request. path_url as part of SpotifyException totally makes sense.

It passes all tests, though I don't believe we have any tests to deal with this specific issue. Perhaps something to add?

Yes integration tests might not consistently test this. If you find a way to unit test this, that'd be amazing, otherwise feel free to submit the PR without it. On my side I'll be testing this manually to make sure it's all good.


Add reference to druid.storage.type

This should be in here. Without setting storage type to S3 globally it will obviously not be used, even if all other parameters are correct.

@jihoonson Done. I was thinking about adding all the possible values of the parameter to the table but since this is S3s documentation page it's probably not that helpful here.

@jihoonson let me know if you agree with the proposed wording and general page structure.


How to merge envs properly? Getting my team-provider.json overwritten.

Amplify CLI Version
12.12.6
Question
Hello, I've been making changes in git branch develop and amplify env develop. I've pushed them both
Now, to merge these changes to the superior branch and env (staging), I've followed these instructions:  here
So, I did git checkout staging, git pull, amplify pull staging. However, when i run 'git merge develop', my team-provider.json gets overriden (the only env appearing is the develop one, the staging is completely empited).
I'm wondering what i am doing wrong and what are the correct procedures to succesfully merge my develop env in the staigng one.

Hey @christiansaavedra, the team-provider-info.json contains the env information and metadata for all environments. Is the staging branch built of the main branch?
If the staging branch is build of the main the team provider should contain the main branch record as well. When git merging you will need to ensure the team provider is appended rather overridden.

Closing the issue due to inactivity. Do reach out to us if you are still experiencing this issue


Viewport draws over editor when zoomed

Operating system or device - Godot version:
current master
Issue description (what happened, and what was expected):
I think this gif speaks for itself:

Steps to reproduce:
Open the splitscreen platformer demo and zoom in on the main scene.

Seems the same as #6172,

Oh, yes it does.
Closing as duplicate then.


Change project structure to be more intuitive

Mostly just renaming folders and creating backend folder structure. Not much done.

{nit}: in the PR add more details, eg: which folders were moved around

{nit}: in the PR add more details, eg: which folders were moved around


色々改善11

#308
を修正しました

014 - Startup Parade!.zip
Using this test chart, the Hard difficulty (including HBSCROLL => NMSCROLL => HBSCROLL transitions) and the Easy difficulty (BMSCROLL => HBSCROLL => NMSCROLL) does not work as expected, only the first scroll method seems to be taken into account


RetroPie System Menu is overriden with blank values

When Scrape all systems is chosen an empty file is created at
home/pi/.emulationstation/gamelists/gamelist.xml
that overrides the default imagery and descriptors for the RetroPie system menu. The names now appear truncated, there are no descriptors, and images are no longer shown.
I request this file not be created.

A note for those in the future that find themselves without a RetroPie "system menu" (the images and descriptors). To repair the gamelist.xml file go to RetroPie Setup
sudo ~/RetroPie-Setup/retropie_setup.sh
reinstall the menu from Manage Packages -> Manage Core Packages -> retropiemenu
The menu will reappear on reboot.


Better user-oriented compiler frontend programs

Right now, if you want to compile a Futhark program, you need to pass the entire pipeline to futhark, pipe the result into a C file, run a C compiler with the right flags, and finally you have a binary.  While this is a very enjoyable process for a compiler developer, it is less so for people who just want to try out Futhark.
I propose we add a futhark-c program to compile freestanding programs using the sequential C backend.  With an option, it will generate just an .o and .h file (for use as a library), and with yet another option, just a .c file.

We now have a better futharki and a new futhark-c.  They need more features and work, but are already much easier to use than old futhark.


feat: spin up multiple chains

#24  👈
main

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

Join @jakim929 and the rest of your teammates on  Graphite


Action config validator 2.0-dev branch

Thanks for opening a Pull Request!
Here's a handy checklist to ensure your PR goes smoothly.

[x] I signed Google's Contributor License Agreement
[x] My code conforms to Google's python style.
[x] My PR at a minimum doesn't decrease unit-test coverage (if applicable).
[x] My PR has been functionally tested.
[x] All of the unit-tests still pass.
[x] Running pylint --rcfile=pylintrc passes.

These guidelines and more can be found in our contributing guidelines.

This change is

Also please add the boilerplate header and a docstring to the init.py files.


Decouple Conductor from Spectator

Hey,
It seems that both Conductor worker and server is tightly coupled with Spectator. Would it make sense to add a metrics interface (extracted from com.netflix.conductor.metrics.Monitors and com.netflix.conductor.client.task.WorkflowTaskMetrics) and make changes to code to this interface ? We can have Spectator as the default implementation.
This is driven by the fact that adopters would want to integrate conductor in their production setups and would want to leverage their existing app monitoring and alerting frameworks within the organization and might not want to invest in the overhead of a prod setup and deployment of Atlas.
Let me know if I am missing something. I would be interested in making the change if it makes sense. Let me know your thoughts.

@cyzhao @v1r3n Will this be a contribution that you guys are willing to consider and is aligned with the goal of adoption ?

@mudit3774 Spectator is already pluggable where any Registry implementation can be plugged in to send metrics to other monitoring systems. Spectator also provides Metrics3 implementation out of box - http://netflix.github.io/spectator/en/latest/registry/metrics3/

Understood. Thanks for the response.


misalign-jal-01.S without support for compressed instruction signature mismatch

The reference model used is Sail, and only the second line of the signature output is different from the DUT.
How to find what is causing this mismatch? All other tests in privilege directory are passing (same signature with ref).
$ diff
< 56ff747d
---
> d6ff74dd

ref.signature.txt
DUT.signature.txt
misalign-jal-01.log.txt

I've been trying to see what is going on. I don't know if the log is the DUT or SAIL, but what is being store in the signature is the delta between the address of the beginning of the test, and a pseudo-random value in a register. The problem seems to be that Sail and the DUT have different values as starting addresses.
This sounds like a test bug, frankly, but I can't be sure unless I can see the other log. I'm a little puzzled that this hasn't turned up earlier, and that is what really odd.

@allenjbaum
The log is SAIL, the DUT does not have such mechanism implemented as SAIL that dumps all the verbose info to a log file.
The issue seems to be this instruction (from ref) address 0x800004E4 - sub a0,a0,sp, I can confirm that a0 here has the same value in DUT and in SAIL, that value is 0x56FF76DD, but sp differs. And yes you're right, the DUT has instructions located in ROM that start from address 0 and data in RAM that start from address 0x80000000.
I am attaching the disassemble for DUT and REF.
ref_disass.txt
dut_disass.txt

It's pretty clear that the test is buggy, and we will have to fix it. What
is more pressing to me is why we haven't caught this previously.
I believe we have tested this with different load points, and it had
passed, and it shouldn't have.
On Thu, Mar 21, 2024 at 3:28 PM ArbnorSh @.***> wrote:

@allenjbaum https://github.com/allenjbaum
The log is SAIL, the DUT does not have such mechanism implemented as SAIL
that dumps all the verbose info to a log file.
The issue seems to be this instruction (from ref) address 0x800004E4 - sub
a0,a0,sp, I can confirm that a0 here has the same value in DUT and in
SAIL, that value is 0x56FF76DD, but sp differs. And yes you're right, the
DUT has instructions located in ROM that start from address 0 and data in
RAM that start from address 0x80000000.
I am attaching the disassemble for DUT and REF.
ref_disass.txt
https://github.com/riscv-non-isa/riscv-arch-test/files/14711873/ref_disass.txt
dut_disass.txt
https://github.com/riscv-non-isa/riscv-arch-test/files/14711874/dut_disass.txt
—
Reply to this email directly, view it on GitHub
https://github.com/riscv-non-isa/riscv-arch-test/issues/445#issuecomment-2013960684,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AHPXVJUGAMY4AOYOEYGQM7TYZNNKLAVCNFSM6AAAAABE2HIS5OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJTHE3DANRYGQ
.
You are receiving this because you were mentioned.Message ID:
@.***>

All implementations I am seeing in github, run these tests with Compressed instruction support. Note in this case DUT does not implement the C extension and Sail is being run with -C or --disable-compressed. If you have any DUT reference, please share that and I can check this test there!

Ah, that would explain it. I feel better now.
On Sat, Mar 23, 2024 at 7:26 AM ArbnorSh @.***> wrote:

All implementations I am seeing in github, run these tests with Compressed
instruction support. Note in this case DUT does not implement the C
extension and Sail is being run with -C or --disable-compressed. If you
have any DUT reference, please share that and I can check this test there!
—
Reply to this email directly, view it on GitHub
https://github.com/riscv-non-isa/riscv-arch-test/issues/445#issuecomment-2016509463,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AHPXVJTJSAAGSYEH7QVRGKDYZWGIBAVCNFSM6AAAAABE2HIS5OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJWGUYDSNBWGM
.
You are receiving this because you were mentioned.Message ID:
@.***>


System.Runtime.CompilerServices.IsReadOnlyAttribute again

I just analyzed a lib which currently targets net472 against ".NET Standard + Platform Extensions,Version=v2.0".
It fails for #ctor in System.Runtime.CompilerServices.IsReadOnlyAttribute, but links no related code in Visual Studios error list like it usually does for other unsupported members.
As there (in this case) were no further warnings and upstream dependencies I was able to just switch from net472 to netstandard2.0 and it compiled fine, even the warning is gone now. System.Runtime.CompilerServices.IsReadOnlyAttribute got compiled as internal class into the target assembly.

The root of this issue is: https://github.com/Microsoft/dotnet-apiport/issues/273. Unfortunately, we don't take into account compiler generated code and skip it.


Deprecate Module.io and BlackBox.io

Why?
I was experimenting with some stuff in Scala 2.13 and realized that -Xsource:2.11 is no longer supported in 2.13. In fact, it was never recommended as an option to Scalac for anything but debugging, let alone our recommended way of supporting Scala 2.12. Thus I had a discussion on the Scala Contributors Gitter to figure out what to do.
For a long time, we thought the issue only had to do with structural typing. This was incorrect, there were actually 2 different problems:

Structural Typing

formerly unsupported in Scala 3, now supported, it always worked in Scala 2.12/13

Type Inference for overridden definitions defaults to overridden type (as opposed to overriding type)

Strict in Scala 3, weaker in Scala 2.12/13

The thing that made this confusing was that fixing either (1) or (2) is sufficient to no longer need -Xsource:2.11, but we generally only talked about solutions to (1). To be more precise, if we stopped using the pattern val io = IO(new Bundle { ... }), we would no longer need -Xsource:2.11 in Scala 2.12.
This solution could work as a ScalaFix rewrite rule:
class MyModule extends Module {
  val io = IO(new Bundle { ... })
}

Becomes
class MyModule extends Module {
  class MyModuleIntf extends Bundle { ... }
  val io = IO(new MyModuleIntf)
}

Unfortunately, due to (2) where Scala 3 is stricter, you would also need to annotate the type of io in Scala 3:
class MyModule extends Module {
  class MyModuleIntf extends Bundle { ... }
  val io: MyModuleIntf = IO(new MyModuleIntf)
}

While this is a solution, there is a massive downside that we have to encourage all of our users to rewrite all of their code (potentially ScalaFix assisted), and purge documentation of this bad style.
It turns out that removing io as a virtual method solves both (1) and (2). The exact same style of Modules with anonymous Bundles for io will work in Scala 2.12, 2.13, and 3.
Implications
What does this actually do?
This deprecation warning only shows up for when io is used as a field of Module or BlackBox itself. That is, only when relying on the common interface of those abstract classes will cause the warning. Some examples:
class Foo extends Module {
  // No warning
  val io = IO(new Bundle {
    val in = Input(Bool())
    val out = Output(Bool())
  })

  io.out := ~io.in // No warning
}

object Util {
  def connect(mod1: Foo, mod2: Foo): Unit = {
    println(s"mod1 has io: ${mod1.io}") // No warning
    mod1.io.in := mod2.io.out           // No warning
  }

  def connect2(mod1: Module, mod2: Module): Unit = {
    println(s"mod1 has io: ${mod1.io}") // Warning!
    mod1.io <> mod2.io                  // Warning!
  }
}

Thus, this warning is actually fairly rare because most people either rely on more type-safe APIs (some common trait defining a more precise type for IO), or use DataMirror.fullModulePorts since they want to be able to get the IO from MultiIOModules and RawModules as well.
Module
The virtual method allowed us to use the type system to enforce the single-source of ports via val io. We can still enforce that through DataMirror.fullModulePorts if we want. We could alternatively unify Module and MultiIOModule and relegate val io to a stylistic choice.
BlackBox
BlackBox has special behavior where the io prefix is dropped. While it's still an open-question for how to provide this behavior in a less hacky way, it needs to be maintained. The io virtual method is currently used to find the ports, but we could use the DataMirror.fullModulePorts "reflective" API to find them and ensure the io prefix is dropped.
Compatibility-mode
import Chisel._ does not require IO wrapping of io for Module and BlackBox. It uses the virtual method to apply the binding to them. 2 possible solutions:

Use reflection to find io
Use the compiler plugin to wrap them

Related issue:

Type of change: other enhancement

Impact: API modification

Development Phase: implementation
Release Notes
Deprecate Module.io and BlackBox.io virtual methods

Wow, I'm pleasantly surprised how simple the solution came out to be (assuming this doesn't break a giant chunk of code that we don't know about yet - though I agree with the arguments presented of why it shouldn't). I think this looks sane.
Other Comments
With regards to alternate solutions: I think it's worth thinking about useful properties of the current (anonymous Bundle) syntax in considering trade-offs. It:

saves a line of class declaration
hints that the Bundle isn't meant to be used elsewhere
and, eliminates the need to come with a class name for a one-of class (and avoids boilerplate names like MyModuleIntf)

Separately from the 2.12+ compatibility issue is whether to unify MultiIOModule and Module - and if this PR goes through, is probably more of a style guide issue. I think the arguments for Module are mainly that it centralizes all the IO in one Bundle, so you can do bulk operations on them (but importantly, this doesn't include clock and reset, but they're usually implicitly connected the same source). However, the issue there is that extending the interface in subclasses becomes a lot harder, since you have to modify io instead of just adding a new val - which is what MultiIOModule makes easierr.
For BlackBox: doesn't ExtModule present a more MultiIOModule like interface, without the need for the .io single object and the naming hackiness? I don't know how much traction / usage it's seen, but it's a bit more elegant, or at least consistent?

Wow, I'm pleasantly surprised how simple the solution came out to be

I cannot express how relieved I am at how simple it came out. This has been a big worry for a long time and it seems we get off basically scot-free.

assuming this doesn't break a giant chunk of code that we don't know about yet

Yeah I think it's important to look out for such issues on 3.4.0-RC1. Also if there is something we don't notice till after full release, it seems pretty easy to undo so I think the risk is low. I did try this with SiFive's codebase and only had like 10 warnings from it and they were all easily fixable cases. For example:
https://github.com/chipsalliance/rocket-chip/blob/2bdb03dbca3f77ad4c378cc1b95ab4961bc1448a/src/main/scala/prci/TestClockSource.scala#L79
The type of source is inferred to be Module with { io : ClockSourceIO }, easily fixable by defining a trait that provides that type for io.
I agree with your Other Comments. I think this is a good reason to just unify Module and MultiIOModule anyway.

For BlackBox: doesn't ExtModule present a more MultiIOModule like interface, without the need for the .io single object and the naming hackiness? I don't know how much traction / usage it's seen, but it's a bit more elegant, or at least consistent?

This one is a little tricker because people like the io stripping behavior of BlackBox. The problem with just flattening everything out in ExtModule is that then you can't just easily bulk connect to things. Imagine a complex AXI interface with dozens of signals that needs to be flattened out, but on the Chisel side you'd like to use a Bundle and be able to bulk connect to the ports of your ExtModule. This is my oft-proposed "prefixless Bundle" API.  In any case, we can think more about this later.