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

Replace bootstrap-conda by grayskull #37447

Open
wants to merge 28 commits into
base: develop
Choose a base branch
from

Conversation

tobiasdiez
Copy link
Contributor

@tobiasdiez tobiasdiez commented Feb 24, 2024

Using the metadata added in #37446, we automatically generate the conda environment files. This no longer makes any reference to the conda.txt files contained in sage-the-distribution. Thus sage-on-top-of-conda is now completely independent of sage-the-distribution (only relying on information specified by sage-the-library). In particular, after this PR is merged the conda.txt files could be deleted from sage-the-distribution.

In particular, we no longer need to maintain the mapping of pypi packages to conda packages but instead can rely on the offical mappings maintained by the conda team (https://github.com/regro/cf-graph-countyfair/tree/master/mappings/pypi).

To test:

pip install https://github.com/conda/grayskull
python tools/update-conda.py

As a byproduct, this fixes #34626.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@mkoeppe mkoeppe added the disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ label Feb 24, 2024
@dimpase dimpase removed the disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ label Feb 26, 2024
@dimpase
Copy link
Member

dimpase commented Feb 26, 2024

I've removed disputed as it was added without a word of explanation. At least if you start a relabelling war, express what exactly you dispute.

@dimpase
Copy link
Member

dimpase commented Feb 26, 2024

do you mean pip install git+https://github.com/conda/grayskull ?

Anyhow, after I do the latter, I get

$ python tools/update-conda.py .
Conda environment file written to src/environment-3.9.yml
Updating lock file for src/environment-3.9.yml at src/environment-3.9-linux
Traceback (most recent call last):
  File "/mnt/opt/Sage/sage-clang/tools/update-conda.py", line 150, in <module>
    update_conda(Path(options.sourcedir))
  File "/mnt/opt/Sage/sage-clang/tools/update-conda.py", line 75, in update_conda
    subprocess.run(
  File "/usr/lib/python3.11/subprocess.py", line 548, in run
    with Popen(*popenargs, **kwargs) as process:
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/subprocess.py", line 1026, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/usr/lib/python3.11/subprocess.py", line 1950, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: 'conda-lock'

What do I do wrong? Does it need to be run in a conda env?

@mkoeppe mkoeppe added the disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ label Feb 26, 2024
@tobiasdiez
Copy link
Contributor Author

What do I do wrong? Does it need to be run in a conda env?

Sorry, forgot that the script uses conda-lock as well. Please try again after pip install conda-lock. Later today, I'll add a bit of documentation to the script.

@dimpase dimpase removed the disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ label Feb 27, 2024
@mkoeppe mkoeppe added the disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ label Feb 27, 2024
@dimpase
Copy link
Member

dimpase commented Feb 28, 2024

Why do I see on an amd64 box

Locking dependencies for ['linux-aarch64']...

while running python tools/update-conda.py . ? Looks like an overkill, no?

@saraedum
Copy link
Member

I took the liberty to add the "needs work" label since it's unclear what's disputed here. (Also, you could maybe check or remove the checklist at the top of the PR to make it clear that there's nothing from that checklist missing here.)

@dimpase

This comment was marked as abuse.

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 17, 2024

@saraedum @roed314 @jhpalmieri I set "disputed" here because when the PR was opened, I was not able to comment because Tobias had blocked me without explanation or giving notice to the sage-abuse committee. It appears that now I can comment, so I am removing "disputed".

There has not been any meaningful review yet here, so conducting a vote would seem premature.

@mkoeppe mkoeppe removed the disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ label Mar 17, 2024
@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 17, 2024

It is correct that on my own PRs, @dimpase is blocked because of his persistent and aggressive code of conduct violations, including the type of denunciations that he has just written above.

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 17, 2024

In any case, this PR here is based on #37446, which is in disputed status because of #37446 (comment)

@dimpase

This comment was marked as abuse.

Copy link
Member

@dimpase dimpase left a comment

Choose a reason for hiding this comment

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

I am giving it a positive review; I tried it out, I pointed a problem, the problem got fixed - despite a rather abusive "There has not been any meaningful review yet here, so conducting a vote would seem premature" comment.

Copy link

Documentation preview for this PR (built with commit 55df9a9; changes) is ready! 🎉

Copy link
Member

@saraedum saraedum left a comment

Choose a reason for hiding this comment

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

I understand that there are reservations about having the pyproject.toml in the root directory. Otherwise, this PR does seem like a reasonable idea to me (replace a shell script with Python and relay some of the heavy lifting to an external project.)

Is there a better place for the pyproject.toml that would make this work for everybody?

@@ -0,0 +1,154 @@
#!/usr/bin/env python3
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be good to add an explanation at the top that explains what this script does. Or maybe add a --help to the argument parser that prints such documentation.

tools/update-conda.py Outdated Show resolved Hide resolved
tools/update-conda.py Outdated Show resolved Hide resolved
return dev_dependencies


update_conda(Path(options.sourcedir))
Copy link
Member

Choose a reason for hiding this comment

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

Not super important but if we put all the global bits of this script in a def main() and do the usual if __name__ == "__main__" or similar, then this could be imported by others to use bits of the machinery for other purposes.

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 20, 2024

Creating a new top-level tools/ directory should probably go through discussion in sage-devel.

And actually, there's no need for a new top-level directory: We already have a place for scripts of exactly this kind -- it is SAGE_ROOT/build/bin/.
In other words, the tools/update-conda.py script should just become part of the sage-bootstrap distribution package.

@saraedum
Copy link
Member

Creating a new top-level tools/ directory should probably go through discussion in sage-devel.

I agree, that script should probably just go elsewhere. But otherwise, if we somehow move the pyproject.toml file, I don't see a problem here.

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 20, 2024

otherwise, if we somehow move the pyproject.toml file, I don't see a problem here.

Same here. This could just be done on top of #37482.

@dimpase
Copy link
Member

dimpase commented Oct 28, 2024

What's happening here?

@tobiasdiez
Copy link
Contributor Author

What's happening here?

It still needs a bit of refinement, but is mostly working and I've used it to create #38878. I'll continue here after #38728 is merged.

In the meantime, it would be helpful if you (or someone else) could review #37446

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

Successfully merging this pull request may close these issues.

Minimize conda environment
5 participants