Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ZCU111 #373

Merged
merged 153 commits into from
Jun 5, 2023
Merged

ZCU111 #373

merged 153 commits into from
Jun 5, 2023

Conversation

rodolfocarobene
Copy link
Contributor

@rodolfocarobene rodolfocarobene commented Apr 11, 2023

This is the qibolab client for multi-qubit-

In particular this should add:

  • Flux support (bias and pulses)
  • Multiplexed readout
    • averaged results
    • single shots
  • Solving everything also for sweepers
    • frequency/amplitude
    • bias
  • Control of LO

Checklist:

  • Reviewers confirm new code works as expected.
  • Tests are passing.
  • Coverage does not decrease.
  • Documentation is updated.

Copy link
Member

@alecandido alecandido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a couple of conversations open, with a simple resolution, but apart from them the rest is good to go (further improvements should be postponed to later PRs).

@rodolfocarobene
Copy link
Contributor Author

Thanks @alecandido , I'll merge as soon tests will pass and I'll receive another approval.
Tests are now failing because it cannot install orjson=3.9.0 that got released 40 minutes ago... I guess it takes some time for the package to be completly available

@alecandido
Copy link
Member

@rodolfocarobene the main problem is that there is not yet any wheel for Linux, nor any source distribution. That's why Ubuntu is failing.

image

These are not generated by PyPI, but by the maintainer that is uploading them. In our, and many other cases, they are generated by a workflow - including orjson.
Something has gone wrong with the workflows, because they mostly completed successfully, including for Linux, but there is yet no wheel...

@scarrazza
Copy link
Member

Downgrading is fine but we could open an issue in their repo.

@alecandido
Copy link
Member

alecandido commented Jun 1, 2023

Downgrading is fine but we could open an issue in their repo.

For sure, I just wanted to see if we could pass. It seems we are not passing, but now for a missing package...

In particular, it's pkg_resources, but I won't investigate further for the time being.

@rodolfocarobene
Copy link
Contributor Author

rodolfocarobene commented Jun 2, 2023

@alecandido I saw that on pypi now there are wheels for orjson 3.9.0 (or at least I think so).
With pip install everything works fine, with poetry install it doesn't work.

A solution, could be to add explicitly in pyproject.toml the dependency on orjson==3.8.14 as an optional needed by zurich (since it's required by laboneq). Locally this solution seems to be working but it is downgrading

Edit:
as you can see, I was wrong :-(

pyproject.toml Outdated Show resolved Hide resolved
@alecandido
Copy link
Member

@rodolfocarobene you're right, wheels are now available, they were simply not present in the lock file.

The current problem is now related to pkg_resources used by dash. It seems that it was a package used to be distributed with setuptools (and, to the best of my understanding, still distributed with it) pypa/setuptools#863
However, we should follow the log from the end, make sure that it's actually dash that uses it, and finally check why is not being installed (or even better, what did change with respect to before).

This might also be a problem that will solve on its own, so I wouldn't be too eager in looking for a solution.

@alecandido alecandido mentioned this pull request Jun 2, 2023
4 tasks
@rodolfocarobene
Copy link
Contributor Author

@alecandido, is adding explicitly setuptools to the pyproject a sensible solution?
I understand that it's not something to do without thinking, but at least the PR is mergable...

Without merging this, both boards are not compatible with main and this is also blocking #459 and the corresponding PR in qibocal. Maybe we could merge as it is, while opening an issue to remember ourselves to remove asap the dependency

@alecandido
Copy link
Member

First you can try, at least for debugging. At the moment, we're not even sure it's going to be a solution (as said, I've just performed a very shallow investigation).

If possible, I'd avoid adding dependency we don't need, and there's also the chance of modifying the lock file (that comes with other complications in the longer term, but it would solve a temporary problem).

Please, if possible, try to understand where the problem has actually been originated. Once we know the source, we can discuss the best course of action.

@rodolfocarobene
Copy link
Contributor Author

If you can see at the workflows, adding setuptools as a dependency solves the problem...
I was not able to find the actual source of the problem... for what I understood also Qibocal had this problem

@alecandido
Copy link
Member

The problem is that Dash has an undeclared dependency on setuptools (importing pkg_resources):
https://github.com/plotly/dash/blob/dev/requires-install.txt
https://github.com/plotly/dash/blob/db23cdcc5fbc33029ffdf1b3083ccc5b2010b3ac/dash/dash.py#L21

In principle, setuptools is available in all environments, but this should not be granted forever, since its unique role has been replaced by the meta-build infrastructure.
Moreover, the appearance of importlib.metadata (in stdlib) made pkg_resources fully obsolete, as described in the docs.

So, the possible (not alternative) routes are:

  1. open an issue in Dash, to notify the problem
  2. understand why it is a problem in the CI (I tried in a fresh environment, but pkg_resources seems to be available in all of them)
  3. add/fix setuptools in the lock file manually (convenient in the short term, since it doesn't require modifying actual files, but inconvenient in the longer one, since the patch should be applied over and over)
  4. put setuptools in pyproject.toml, like already did (but also check the lockfile to understand actual differences)

@rodolfocarobene
Copy link
Contributor Author

I think I understood the problem (maybe, ahaha).

As you can see in the failing action poetry explicitly remove setuptools from the environment when executing poetry install --no-interaction --no-root. This does not happen anywhere else in the repo and the only real difference is the qibosoq dependency. Removing it, solves the issue (but clearly is not a viable option).

The poetry.lock file has a notable difference when qibosoq is added: it has explicitly written a setuptools dependency:

qibolab/poetry.lock

Lines 4363 to 4368 in fc8b0fe

[[package]]
name = "setuptools"
version = "67.8.0"
description = "Easily download, build, install, upgrade, and uninstall Python packages"
category = "main"
optional = true

The dependency is added because qibosoq depends on qick that depends on pynq that depends on setuptools.
I think that since it is explicitly written that setuptools is optional, then it is removed with poetry install --no-interaction --no-root.

The problem, then, is that poetry does not re-install it with poetry install --no-interaction --with docs,tests,analysis --all-extras because the chain of dependencies is broken by pynq that does not get installed because qick has the dependency just in "board" architectures:

pynq = {version = ">=2.6", markers = "platform_machine == \"aarch64\" or platform_machine == \"armv7l\""}

So setuptools gets removed because it's optional, but then does not get re-installed, thus causing the error.

I already opened an issue in the dash repository suggesting to add the dependency on setuptools or to stop using pkg_resources, but in the meantime I think it's better to add an explicit setuptools dependency in the pyproject.toml (eventually we could also open an issue to remember us to remove it).

I'm not sure which is the best section to add it, if in the general section [tool.poetry.dependencies] as not optional or as an optional package required by qm since dash is required just by qualang-tools.

@alecandido
Copy link
Member

alecandido commented Jun 5, 2023

Thanks for plotly/dash#2557, it is perfectly detailed :)

And also thanks for the great investigation. I'm not perfectly sure about the dynamics according to which setuptools gets removed from the env by Poetry, but your explanation is at least convincing.

The current solution is fine, most likely one of the best we can achieve on our side. And I agree about the issue in Qibolab: write it, and link to the corresponding dash issue (the one you opened).
As soon as the original one gets addressed, we can keep pushing (we might also have to go through qm, asking them to upgrade their dash dependency, but if they did not specify explicit version constraints maybe it won't be required).

The only bit I'm still missing (but we don't have to solve) is why this problem has been triggered by this PR. The current explanation would suggest it could apply also to main...

@rodolfocarobene
Copy link
Contributor Author

But in main there is still no dependency on qibosoq!
In any case, thanks for the approval, I'll proceed to merge

@rodolfocarobene rodolfocarobene merged commit 4767ca5 into main Jun 5, 2023
@rodolfocarobene rodolfocarobene deleted the zcu111-server branch June 5, 2023 08:49
@alecandido
Copy link
Member

You're right, I forgot for a moment that the problem was supposed to come from pynq.

Thanks for the PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants