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

Feature/covalent #196

Closed
wants to merge 22 commits into from
Closed

Feature/covalent #196

wants to merge 22 commits into from

Conversation

satyaog
Copy link
Member

@satyaog satyaog commented Feb 21, 2024

milabench cloud --setup

It creates a system config file and takes a target cloud platform with --run-on.

This starts a local covalent server which is used to manage python code that will be executed on the remote. For now this is only somewhat useful since milabench is mostly using ssh commands anyway and it would take a bit of time to refactor the pipeline I think to instead use the covalent interface to run code. I think this could be an interesting approach but it's a nice to have for now.

So milabench cloud --setup setup the remote and install basic stuff on it like the correct python version (necessary to ensure good serialization/deserialization of python objects between the local and remote machine), pip and venv. venv is used to separate the covalent env and milabench env which have incompatible package requirements versions (sqlalchemy caused problems). On this is done , the covalent server becomes useless

Then system config file should be used in the install, prepare and run commands. In those commands it creates a new standalone config for the tests that will be executed and copies it to the remote before the rest of the pipeline is executed.

At the end of the run command the results are copied to the local machine to allow the generation of a report

At the very end, milabench cloud --teardown should be used to release the cloud resources. The --all argument will release all resources of a target cloud platform specified with --run-on.

milabench report --push

Push the results to a reports branch which as well stores the status svg and summary

Example of reports : #210

@satyaog satyaog changed the base branch from master to stable February 21, 2024 22:42
@Delaunay Delaunay changed the base branch from stable to master February 23, 2024 16:05
@Delaunay Delaunay changed the base branch from master to stable February 23, 2024 16:08
@satyaog satyaog force-pushed the feature/covalent branch 5 times, most recently from 53c00e3 to ac6b421 Compare February 27, 2024 14:58
@satyaog satyaog force-pushed the feature/covalent branch 3 times, most recently from dcad77a to 90cad06 Compare March 8, 2024 14:52
@Delaunay Delaunay changed the base branch from stable to master March 11, 2024 18:22
@satyaog satyaog marked this pull request as ready for review March 14, 2024 21:28
@satyaog satyaog force-pushed the feature/covalent branch 3 times, most recently from f6fc50d to ba6f280 Compare March 15, 2024 19:56
covalent is not compatible with milabench as it requires sqlalchemy<2.0.0
Comment on lines 43 to 44
with:
token: ${{ secrets.REPORTS_PAT }}
Copy link
Member Author

Choose a reason for hiding this comment

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

I failed to make this work to be able to push to the reports branch

Suggested change
with:
token: ${{ secrets.REPORTS_PAT }}


- name: Summary
run: |
git remote set-url origin "https://${{ vars.REPORTS_USERNAME }}:${{ secrets.REPORTS_PAT }}@$(git remote get-url origin | cut -d'/' -f3-)"
Copy link
Member Author

Choose a reason for hiding this comment

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

There's probably a better way to do this with actions/checkout@v3

milabench/cli/badges/requirements.txt Outdated Show resolved Hide resolved
milabench/cli/covalent/requirements.txt Outdated Show resolved Hide resolved
**_get_executor_kwargs(args),
)

def _popen(cmd, *args, _env=None, **kwargs):
Copy link
Member Author

Choose a reason for hiding this comment

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

This should probably be simplified, I don't think we'll use the complex features on configs and args paths

milabench/common.py Outdated Show resolved Hide resolved
Comment on lines 372 to 378
reports_url = ([
_r.url for _r in _repo.remotes if "mila-iqia" in _r.url
] or [
_r.url for _r in _repo.remotes if _r.name == "origin"
])[0]
reports_url = XPath("github.com".join(reports_url.split("github.com")[1:])[1:])
reports_url = XPath("https://github.com") / f"{reports_url.with_suffix('')}/tree/{reports_repo.active_branch.name}"
Copy link
Member Author

Choose a reason for hiding this comment

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

This might not be needed if we set the link with markdown instead of the svg itself to redirect to the summary page

Comment on lines 399 to 403
tag = ([
t.name
for t in _repo.tags
if meta[0]["milabench"]["tag"].startswith(t.name)
] or [meta[0]["milabench"]["tag"]])[0]
Copy link
Member Author

Choose a reason for hiding this comment

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

I kind of feel that using the tag as is would be better even if there's a chance I think it will duplicate the versions? Let me know

Suggested change
tag = ([
t.name
for t in _repo.tags
if meta[0]["milabench"]["tag"].startswith(t.name)
] or [meta[0]["milabench"]["tag"]])[0]
tag = meta[0]["milabench"]["tag"]

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is this

I think it would be clearer if we use "branch-commit" because the tag means actually nothing in most cases.
For example, in master the latest tag is still v0.0.6 but stable latest tag is v0.0.10.

@satyaog satyaog force-pushed the feature/covalent branch 5 times, most recently from c486705 to 1991c52 Compare May 22, 2024 18:39
@satyaog satyaog force-pushed the feature/covalent branch 3 times, most recently from f49c60a to 0cb1bcd Compare May 22, 2024 19:17
@satyaog satyaog temporarily deployed to test-cloud-ci-dne May 22, 2024 19:19 — with GitHub Actions Inactive
@satyaog
Copy link
Member Author

satyaog commented May 22, 2024

The workflow here cannot access repo's secrest even if I'm an admin. Seams related to this explanation https://stackoverflow.com/a/58740879. Closing in favour of #217 which is a local branch

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.

2 participants