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

NEW profiler decorator #1110

Merged
merged 7 commits into from
Aug 18, 2020
Merged

NEW profiler decorator #1110

merged 7 commits into from
Aug 18, 2020

Conversation

viniciusdc
Copy link
Contributor

This should enable us to inspect more functions with more freedom.

@CJ-Wright
Copy link
Member

Can you implement this on the current jobs?

@viniciusdc
Copy link
Contributor Author

viniciusdc commented Aug 14, 2020

Can you implement this on the current jobs?

Yes, it is possible. Give me some minutes and I will create the PR's

@viniciusdc
Copy link
Contributor Author

viniciusdc commented Aug 14, 2020

I will implement this on the update_prs profiler stuff, this way I can test it in a better way. If goes well we should merge this and use the @profiling() instead

@codecov
Copy link

codecov bot commented Aug 14, 2020

Codecov Report

Merging #1110 into master will increase coverage by 0.03%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1110      +/-   ##
==========================================
+ Coverage   58.72%   58.76%   +0.03%     
==========================================
  Files          52       57       +5     
  Lines        5018     5194     +176     
==========================================
+ Hits         2947     3052     +105     
- Misses       2071     2142      +71     
Impacted Files Coverage Δ
conda_forge_tick/auto_tick.py 0.00% <0.00%> (ø)
conda_forge_tick/make_graph.py 0.00% <0.00%> (ø)
conda_forge_tick/profiler.py 0.00% <0.00%> (ø)
conda_forge_tick/update_prs.py 0.00% <0.00%> (ø)
conda_forge_tick/contexts.py 67.92% <0.00%> (-1.72%) ⬇️
tests/test_mamba_solvable.py 85.71% <0.00%> (-0.50%) ⬇️
conda_forge_tick/migrators/arch.py 26.24% <0.00%> (-0.23%) ⬇️
tests/test_migration_yaml_migration.py 93.33% <0.00%> (-0.15%) ⬇️
tests/test_migrators.py 93.85% <0.00%> (-0.11%) ⬇️
conda_forge_tick/utils.py 58.53% <0.00%> (-0.11%) ⬇️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3962f90...e31b4f8. Read the comment docs.

@viniciusdc
Copy link
Contributor Author

viniciusdc commented Aug 14, 2020

There is a problem with using function_name = f.__name__ to get the process name (for the sub-folders),cause we are actually using it on the main functions so... it is gonna be a mess. I will replace it with

# process name -- aka profiler sub-folder
process_name = os.path.basename(__file__)
# check dir
os.makedirs(f"profiler/{process_name}", exist_ok=True)

Then it should work like this, at update_prs.py for example, the output profiler file should be named asprofiler/update_prs/{current_time}.I will run a local test to check if it works as announced

@viniciusdc
Copy link
Contributor Author

Local test passed

@viniciusdc
Copy link
Contributor Author

Also, after #1111 works, we could rethink this #1085, right ?

@CJ-Wright
Copy link
Member

yes

@viniciusdc
Copy link
Contributor Author

As commented by @CJ-Wright there is no need for two PR's with the same goal, then I will be closing #1111 as this PR now comprehends those changes too (in this case as a test for the new @profiling())

@viniciusdc viniciusdc requested a review from CJ-Wright August 17, 2020 19:45
@viniciusdc
Copy link
Contributor Author

Can you implement this on the current jobs?

Once we test it with the update_prs I can insert the on the other process

@viniciusdc
Copy link
Contributor Author

Once tested and working well, we should go to #1085

@viniciusdc viniciusdc mentioned this pull request Aug 17, 2020
@CJ-Wright
Copy link
Member

Is this decorator being applied to the auto tick runs as well?

@viniciusdc
Copy link
Contributor Author

Is this decorator being applied to the auto tick runs as well?

not yet

@viniciusdc
Copy link
Contributor Author

viniciusdc commented Aug 17, 2020

I want to confirm that it will work as expected, here it goes well but there is always an unexpected thing that can happen

@viniciusdc
Copy link
Contributor Author

viniciusdc commented Aug 17, 2020

If it works, I will update the autotick with @profiling() and work on the limit stuff (there is so much 'old' information there, only consuming space)

@CJ-Wright
Copy link
Member

Do you need this PR merged first?

@viniciusdc
Copy link
Contributor Author

viniciusdc commented Aug 17, 2020

Do you need this PR merged first?

Yes, if possible. The order should be something like:

  1. This PR with the profile.py and the update_prs (as a test);
  2. Update auto_tick profiler using the @profiling();
  3. Remove old instances of profiled files.

I also can insert the update of auto_tick here instead, this way we don't need to stop it twice, what d you think ?

@CJ-Wright
Copy link
Member

It would be good if all the changes related to the profiler decorator could be done in one PR.

@viniciusdc
Copy link
Contributor Author

It would be good if all the changes related to the profiler decorator could be done in one PR.

For that, I must figure out a way to remove the oldest files from profiler/*, do you have any idea ?

@CJ-Wright
Copy link
Member

I would leave that portion out for the moment, since formally it isn't about the decorator, also that requires manual changes to the database.

@viniciusdc
Copy link
Contributor Author

I would leave that portion out for the moment, since formally it isn't about the decorator, also that requires manual changes to the database.

yup, Y 'r right. So for the profiling that's all here. Unless there is other part of the bot I should add the decorator.

@CJ-Wright
Copy link
Member

You could put it on make graph, that might be interesting.

@CJ-Wright CJ-Wright merged commit b6897cf into regro:master Aug 18, 2020
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