Expose wd via scheduler

[b-bondurant]

ARTIQ Pull Request

Description of Changes

Add get_wd(rid) method to scheduler and scheduler virtual device. For the use case I have in mind, this would retrieve the repository path when using a filesystem backend, but it could also be useful for retrieving the path of the tmpdir when using the git backend.

Hardly a "feature," but not technically a bugfix either.

Signed-off-by: Brad Bondurant brad.bondurant@duke.edu

Type of Changes

	Type
✓	✨ New feature

Steps (Choose relevant, delete irrelevant before submitting)

All Pull Requests

• Use correct spelling and grammar.
• Update RELEASE_NOTES.rst if there are noteworthy changes, especially if there are changes to existing APIs.
• Check the copyright situation of your changes and sign off your patches (git commit --signoff, see copyright).

This does add to the scheduler API, but I wasn't sure if it was significant enough to warrant updating the release notes.

Code Changes

• Run flake8 to check code style (follow PEP-8 style). flake8 has issues with parsing Migen/gateware code, ignore as necessary.
• Test your changes or have someone test them. Mention what was tested and how.
• Add and check docstrings and comments
• Check, test, and update the unittests in /artiq/test/ or gateware simulations in /artiq/gateware/test

Current unit tests pass, no new tests added. Manual testing done with filesystem backend, returns path to repository as expected.

Git Logistics

• Split your contribution into logically separate changes (git rebase --interactive). Merge/squash/fixup commits that just fix or amend previous commits. Remove unintended changes & cleanup. See tutorial.
• Write short & meaningful commit messages. Review each commit for messages (git show). Format:

  topic: description. < 50 characters total.
  
  Longer description. < 70 characters per line
  

Licensing

See copyright & licensing for more info.
ARTIQ files that do not contain a license header are copyrighted by M-Labs Limited and are licensed under LGPLv3+.

[airwoodix]

I unfortunately cannot test this right now: what's the difference between wd and dirname(expid["file"])?
Given #1543, adding the canonical usage to the docs would be a great improvement IMHO.

[airwoodix]

wd should be spelled out if this is part of the user-facing API. Attributes of artiq.master.scheduler.Run are not documented.

[airwoodix]

Matter of taste: why not?

try:
    return pipeline.pool.runs[rid].wd
except KeyError:
    raise KeyError("RID not found")  # [from None]

[b-bondurant]

I was going for consistency with check_pause(), but yeah there's no reason why not

[b-bondurant]

Actually, if you put this whole try/except inside the loop then I don't think it would work since this would raise a key error for every pipeline that doesn't contain that RID, even if there's one that does.

[airwoodix]

Good point indeed, sorry. Missed one level of indentation.

[b-bondurant]

Probably a moot point anyway as it looks like this PR will be rejected for release-5, and unnecessary for master/future release-6

[b-bondurant]

what's the difference between wd and dirname(expid["file"])

If the file is in-repository, then expid['file'] is a path relative to the repository root. Before the experiment is imported, that path is joined with wd to get the full absolute path to the file. If the file is out of repo, then it's just the absolute path to the file (and there is no repo or wd to speak of).

Given #1543, adding the canonical usage to the docs would be a great improvement IMHO.

I hadn't seen that issue, but I agree that this might be helpful there, and it's actually pretty similar to the use case I have in mind, although I'm not using the git backend. I'm writing a sort of base class that needs to import other experiments (specified by the user when they implement the class) based on a file and class_name, and I decided to use artiq.tools.file_import and .get_experiment - but unfortunately I need absolute paths since the repository root path isn't directly exposed anywhere AFAICT. For the use case in that issue, it seems like they could use the same artiq.tools if they knew the directory of the git checkout (which I think is what wd would be in that case).

[b-bondurant]

An alternate implementation of this functionality would be to simply add a wd field to the run's notifier - that might actually be more favorable as it's less intrusive and doesn't technically change the API.

[sbourdeauducq]

After #1543 (comment) we could just do os.getcwd() in experiments I think, without requiring this relatively complex change?

[b-bondurant]

Yes! If the working directory was set to the repository rather than the results directory, I think that would take care of things. Is that a change you plan on introducing into release-5 as well?

[sbourdeauducq]

Is that a change you plan on introducing into release-5 as well?

No, and this PR would not be eligible for release-5 either.

[b-bondurant]

An alternate implementation of this functionality would be to simply add a wd field to the run's notifier - that might actually be more favorable as it's less intrusive and doesn't technically change the API.

Would you consider it for release-5 if I went with this approach instead? Just to make do until we're ready to update to the next release.

[sbourdeauducq]

No, we don't introduce new features on release-5 either.
If you're using Nix, you can use overrideAttrs and patches to cleanly apply the change (which would be #1543 (comment) to end up in master) to your local ARTIQ-5 installation.

[b-bondurant]

Understood, thanks!

[lriesebos]

@sbourdeauducq I see you have reopened this PR a while ago. Is there a way @b-bondurant can refactor this change to get this functionality into master at some point?

[sbourdeauducq]

Do you still need it if we do #1543?

[b-bondurant]

Do you still need it if we do #1543?

I think we would, but I'm not familiar enough with Python's import machinery to be sure.

My specific use case is a bit different from those that have been discussed so far in that issue. I'm using artiq.tools.file_import for runtime imports in a class I wrote for DAX. It's a bit complicated to explain, so I'll just give a brief summary and then link to the relevant code.

Essentially, this class (CalibrationJob) receives the paths (absolute or relative to the repository directory) and class names of two experiments via class variables from a user-implemented subclass, then wraps them into a single "meta" experiment and inserts that experiment into the global namespace of the file containing the CalibrationJob subclass so that it may be picked up by ARTIQ as part of the repository.

class def: https://gitlab.com/duke-artiq/dax/-/blob/master/dax/base/scheduler.py#L693
runtime imports: https://gitlab.com/duke-artiq/dax/-/blob/master/dax/base/scheduler.py#L827-830
current hack to resolve repository-relative paths: https://gitlab.com/duke-artiq/dax/-/blob/master/dax/base/scheduler.py#L1068-1077
decorator to create meta experiment at import time and insert into user file: https://gitlab.com/duke-artiq/dax/-/blob/master/dax/base/scheduler.py#L1112

[sbourdeauducq]

@b-bondurant Since we didn't reach consensus on an alternative approach, let's merge this. Can you address the review items?

[lriesebos]

Maybe it is possible to rename the function to get_repository() to leave room for #1747 ?

[b-bondurant]

Maybe it is possible to rename the function to get_repository() to leave room for #1747 ?

@sbourdeauducq what do you think about something like that? It also might be clearer to users since (as @airwoodix pointed out), wd isn't documented, and AFAIK everywhere else in the docs it's referred to as "repository". Also, given recent PR #1767, it's possible users would expect get_wd to point to the subdirectory, while calling it get_repository_dir would (IMO) remove any potential confusion.

edit: just to be clear, I ran a local test and get_wd returns the repository directory even if the master was started with an --experiment-subdir specified.

[sbourdeauducq]

What happens with experiments submitted "outside the repository"? This corner case is also what is plaguing the other proposals.

[b-bondurant]

It returns None, which I've explained in my updated docstring (unpushed since I'm planning to amend and force push to make the switch to get_repository_dir if we decide that's what we want to call it)

[b-bondurant]

And I actually didn't need to do any special handling for that case since I'm just returning run.wd, which gets set to None by default if there's no repo_rev field in the expid, i.e. for out-of-repo experiments.

[b-bondurant]

For git-based repositories (i.e. using artiq_master -g ...), it returns the temp dir containing the checkout that the master is using. I think that would be the desired behavior, since getting the path of a bare git repo would be useless, but I'll make sure to note that in the docstring.