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

👷 Switch to uv for CI #1518

Merged
merged 19 commits into from
Mar 25, 2024
Merged

👷 Switch to uv for CI #1518

merged 19 commits into from
Mar 25, 2024

Conversation

Zethson
Copy link
Member

@Zethson Zethson commented Mar 24, 2024

Working on this while being in calls

Signed-off-by: zethson <[email protected]>
@Zethson
Copy link
Member Author

Zethson commented Mar 24, 2024

    # now, we have the associated artifacts
    transform = ln.Transform.filter(uid="hlsFXswrJjtt5zKv").one_or_none()
    assert transform is not None
    assert transform.latest_report.path.exists()
    assert transform.latest_run.report.path == transform.latest_report.path
  assert transform.source_code.hash == "bH9mTpWerQcoI0UcDIkKSw"

E AssertionError: assert 'yv9kLPg8GRohJTgDZ7Zsyw' == 'bH9mTpWerQcoI0UcDIkKSw'
E
E - bH9mTpWerQcoI0UcDIkKSw
E + yv9kLPg8GRohJTgDZ7Zsyw

not quite sure why this test suddenly fails

Signed-off-by: zethson <[email protected]>
@Zethson
Copy link
Member Author

Zethson commented Mar 24, 2024

Wild idea: Because the Python kernel changed which changes the notebook hash.

@Koncopd
Copy link
Member

Koncopd commented Mar 24, 2024

Not wild, it is very probably what happened.

Signed-off-by: zethson <[email protected]>
Copy link

codecov bot commented Mar 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.03%. Comparing base (7ffed2e) to head (f7bf67f).
Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1518      +/-   ##
==========================================
- Coverage   93.13%   93.03%   -0.10%     
==========================================
  Files          52       52              
  Lines        4862     4981     +119     
==========================================
+ Hits         4528     4634     +106     
- Misses        334      347      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: zethson <[email protected]>
Signed-off-by: zethson <[email protected]>
Signed-off-by: zethson <[email protected]>
Signed-off-by: zethson <[email protected]>
@chaichontat
Copy link
Contributor

Note: I cache submodule installs in my CI runs, along with all dependencies. This would save more time here as well.

https://github.com/laminlabs/laminapp-ui/blob/8ff8fa8f3ea0b17106801d7f885d5d9105c0894a/.github/workflows/setup/action.yml#L12

@Zethson
Copy link
Member Author

Zethson commented Mar 24, 2024

Not sure whether hitting the cache is worth for the clones, but we can try! Thanks @chaichontat

@falexwolf
Copy link
Member

Exactly! Maybe a reason for storing source code for notebooks as .py and ditching all these unnecessary metadata that mess with hashes.

Copy link

github-actions bot commented Mar 25, 2024

@github-actions github-actions bot temporarily deployed to pull request March 25, 2024 09:00 Inactive
@falexwolf
Copy link
Member

falexwolf commented Mar 25, 2024

Building the docs in non-strict mode gives these strange warnings:

image

And these docs:

https://66013d3de8ba209aea9a6083--lamindb-qnwk.netlify.app/guide.html

These 4 come from the transform types:

image

https://github.com/laminlabs/lnschema-core/blob/8e2fe928b8d27e685cae9c8f76076021a52cfe4f/lnschema_core/types.py#L34

🤔

@falexwolf
Copy link
Member

@falexwolf
Copy link
Member

The sphinx code that renders TransformType in my local env looks like so:

lamindb.core.types.TransformType
================================

.. currentmodule:: lamindb.core.types

.. autoclass:: TransformType
   :show-inheritance:

Hence, no mention of modules, etc.

I suspect that a more recent docs dependency now renders the Enum differently, and leads to the error.

@falexwolf falexwolf changed the title Switch to uv for CI 👷 Switch to uv for CI Mar 25, 2024
@github-actions github-actions bot temporarily deployed to pull request March 25, 2024 09:36 Inactive
@falexwolf
Copy link
Member

falexwolf commented Mar 25, 2024

I'm also installing the docs dependencies with uv now, but the error persists.

Looking at dependencies of the last successful build:

Successfully installed MarkupSafe-2.1.5 ablog-0.10.25 alabaster-0.7.16 apeye-1.4.1 apeye-core-1.1.5 autodocsumm-0.2.12 babel-2.14.0 beautifulsoup4-4.12.3 cachecontrol-0.14.0 cssutils-2.9.0 dict2css-0.3.0.post1 dirsync-2.2.5 docutils-0.19 domdf-python-tools-3.8.0.post2 feedgen-1.0.0 greenlet-3.0.3 html5lib-1.1 imagesize-1.4.1 importlib_metadata-7.1.0 invoke-2.2.0 jinja2-3.1.3 jupyter-cache-0.6.1 lndocs-0.3.0 lxml-5.1.0 markdown-3.6 markdown-it-py-2.2.0 mdit-py-plugins-0.3.5 msgpack-1.0.8 myst-nb-0.17.2 myst-parser-0.18.1 nbclient-0.7.4 pydata-sphinx-theme-0.12.0 ruamel.yaml-0.18.6 ruamel.yaml.clib-0.2.8 snowballstemmer-2.2.0 soupsieve-2.5 sphinx-5.3.0 sphinx-autodoc-typehints-1.23.0 sphinx-copybutton-0.5.2 sphinx-design-0.5.0 sphinx-jinja2-compat-0.2.0.post1 sphinx-prompt-1.5.0 sphinx-tabs-3.4.5 sphinx_toolbox-3.5.0 sphinxcontrib-applehelp-1.0.8 sphinxcontrib-devhelp-1.0.6 sphinxcontrib-htmlhelp-2.0.5 sphinxcontrib-jsmath-1.0.1 sphinxcontrib-qthelp-1.0.7 sphinxcontrib-serializinghtml-1.1.10 sphinxext-opengraph-0.9.1 sqlalchemy-2.0.28 tabulate-0.9.0 watchdog-4.0.0 webencodings-0.5.1 zipp-3.18.1

Versus here (pip install that first failed):

MarkupSafe-2.1.5 ablog-0.10.25 alabaster-0.7.16 apeye-1.4.1 apeye-core-1.1.5 autodocsumm-0.2.12 babel-2.14.0 beautifulsoup4-4.12.3 cachecontrol-0.14.0 cssutils-2.9.0 dict2css-0.3.0.post1 dirsync-2.2.5 docutils-0.19 domdf-python-tools-3.8.0.post2 feedgen-1.0.0 greenlet-3.0.3 html5lib-1.1 imagesize-1.4.1 importlib_metadata-7.1.0 invoke-2.2.0 jinja2-3.1.3 jupyter-cache-0.6.1 lndocs-0.3.0 lxml-5.1.0 markdown-3.6 markdown-it-py-2.2.0 mdit-py-plugins-0.3.5 msgpack-1.0.8 myst-nb-0.17.2 myst-parser-0.18.1 nbclient-0.7.4 pydata-sphinx-theme-0.12.0 ruamel.yaml-0.18.6 ruamel.yaml.clib-0.2.8 snowballstemmer-2.2.0 soupsieve-2.5 sphinx-5.3.0 sphinx-autodoc-typehints-1.23.0 sphinx-copybutton-0.5.2 sphinx-design-0.5.0 sphinx-jinja2-compat-0.2.0.post1 sphinx-prompt-1.5.0 sphinx-tabs-3.4.5 sphinx_toolbox-3.5.0 sphinxcontrib-applehelp-1.0.8 sphinxcontrib-devhelp-1.0.6 sphinxcontrib-htmlhelp-2.0.5 sphinxcontrib-jsmath-1.0.1 sphinxcontrib-qthelp-1.0.7 sphinxcontrib-serializinghtml-1.1.10 sphinxext-opengraph-0.9.1 sqlalchemy-2.0.29 tabulate-0.9.0 watchdog-4.0.0 webencodings-0.5.1 zipp-3.18.1

I don't find any difference. It's exactly the same environment.

@falexwolf
Copy link
Member

Hence, the reason is not the docs dependencies, but the upstream Python dependencies. Docs in Sergei's PR are passing, too.

uv must be installing something that breaks sphinx documentation for Enums...

@falexwolf
Copy link
Member

Is it just Python 3.11?
image

@github-actions github-actions bot temporarily deployed to pull request March 25, 2024 10:00 Inactive
@falexwolf
Copy link
Member

It seems this was it. Back to strict mode.

@Zethson
Copy link
Member Author

Zethson commented Mar 25, 2024

Sad - Python 3.11 might give us another tiny speedup ^_^
But all right then...

@github-actions github-actions bot temporarily deployed to pull request March 25, 2024 10:26 Inactive
@Zethson
Copy link
Member Author

Zethson commented Mar 25, 2024

def build_docs(session, strict: bool = False, strip_prefix: bool = False):
prefix = "." if Path("./lndocs").exists() else ".."
session.run(*f"uv pip install --system {prefix}/lndocs".split())
# do not simply add instance creation here
args = ["lndocs"]
if strict:
args.append("--strict")
if strip_prefix:
args.append("--strip-prefix")
session.run(*args)

Do you want to keep this local or should I add a uv flag in laminci.nox? Then we could gradually transition to uv for all of our repositories

@falexwolf
Copy link
Member

Do you want to keep this local or should I add a uv flag in laminci.nox? Then we could gradually transition to uv for all of our repositories

We'll backport to laminci once the transition has been made everywhere.

If we do it now, we'll break all repos because they don't come with uv install.

@Zethson
Copy link
Member Author

Zethson commented Mar 25, 2024

Do you want to keep this local or should I add a uv flag in laminci.nox? Then we could gradually transition to uv for all of our repositories

We'll backport to laminci once the transition has been made everywhere.

If we do it now, we'll break all repos because they don't come with uv install.

That's why I suggested a flag -> run_with_uv: bool = False

@falexwolf
Copy link
Member

Ideally, we install uv by default through laminci just like we default nox, then we could backport right away.

@Zethson Zethson marked this pull request as ready for review March 25, 2024 10:33
@falexwolf
Copy link
Member

@falexwolf
Copy link
Member

falexwolf commented Mar 25, 2024

run_with_uv

Sorry, I didn't see that you suggested. But adding uv as a dependency to laminci will ensure that all repos keep working, and then we don't need to add a new flag.

@Zethson
Copy link
Member Author

Zethson commented Mar 25, 2024

On it

@Zethson
Copy link
Member Author

Zethson commented Mar 25, 2024

@falexwolf could you make a laminci release, please? laminlabs/laminci#34

@falexwolf
Copy link
Member

falexwolf commented Mar 25, 2024

I just put laminci 0.12.0 on pypi!

@falexwolf
Copy link
Member

falexwolf commented Mar 25, 2024

Installing laminci only took 5s also before (and maybe we can get rid of more dependencies in its default install over time).

image

@github-actions github-actions bot temporarily deployed to pull request March 25, 2024 11:11 Inactive
Signed-off-by: zethson <[email protected]>
@github-actions github-actions bot temporarily deployed to pull request March 25, 2024 11:19 Inactive
noxfile.py Show resolved Hide resolved
@falexwolf
Copy link
Member

Otherwise, great! Can be merged!

@Zethson Zethson merged commit fe5f89c into main Mar 25, 2024
12 of 13 checks passed
@Zethson Zethson deleted the feature/uv branch March 25, 2024 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants